Fixed nullity discreapency between documenation and assert check in R… #18076
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
…egexRequestMatcher Signed-off-by: Soumik Sarker <[email protected]>
ronodhirSoumik
force-pushed
the
bugfix/fixed-nullity-documentation-mismatch-regexRequestMatcher
branch
from
c2b6652 to
6975c70
Compare
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: |
4 participants
Related to #18069