Fixed nullity discreapency between documenation and assert check in R…#18076
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
ronodhirSoumik
force-pushed
the
bugfix/fixed-nullity-documentation-mismatch-regexRequestMatcher
branch
from
c2b6652 to
6975c70
Compare
jzheaux
left a comment
jzheaux
left a comment
There was a problem hiding this comment.
Hi, @ronodhirSoumik! Thanks for this cleanup. I've left some feedback inline.
| /** | ||
| * Creates a case-sensitive {@code Pattern} instance to match against the request. | ||
| * @param method the HTTP method to match. May be null to match all methods. | ||
| * @param method the HTTP method to match. May not be null to match all methods. |
There was a problem hiding this comment.
May not be null to match all methods.
This sentence now sounds a little cryptic. Can we simply leave it out? Folks can call another static method if they only care about the URI.
There was a problem hiding this comment.
While Javadoc can be improved, I don't see a good reason to restrict input value here artificiallly. Constructor already takes care of the null case; throughout the class httpMethod is assumed to be nullable. Just let it be null. A sibling PathPatternRequestMatcher, for example, allows method as null, explicitly marking it as @Nullable. While it would be good to get rid of nullable all together, HttpMethod does not have an "any/all" option.
Ideally, a builder similar to one in PathPatternRequestMatcher would do a good job here. Meanwhile, I'm just using constructor, because I get the method as an input myself, so checking null and deciding between faactory methods is much more verbose then just calling a constructor.
There was a problem hiding this comment.
I am undoing the change in javadoc and removing the Assert.notNull(method, "method cannot be null"); - hope that do as a minimal change
|
Also, @ronodhirSoumik, will you please reference #18069 in your commit message. Something like this: |
ronodhirSoumik
force-pushed
the
bugfix/fixed-nullity-documentation-mismatch-regexRequestMatcher
branch
2 times, most recently
from
bb27474 to
1ee78dd
Compare
…tcher Closes spring-projectsgh-18069 Signed-off-by: Soumik Sarker <ronodhirsoumik@gmail.com>
ronodhirSoumik
force-pushed
the
bugfix/fixed-nullity-documentation-mismatch-regexRequestMatcher
branch
from
1ee78dd to
b6b23d8
Compare
|
Hi @jzheaux @edudar-chwy, can you please review whether this PR is all good? |
4 participants
Related to #18069