added support for Optional Request Parameters #550
Conversation
- Adds tests that were missing in HelloControllerTest that actually exercises the feature
- Fixes the controller URLs removing incorrect {myOptional} from urls
- Fixes PathTypeConversion to handle null values
- Adds missing javadoc on new public methods of PathTypeConversion
- Adds some useful unit tests for TypeMap.OptionalHandler
|
Thanks for the PR, nice work. Note that I have submitted a PR to fix the issues/bugs that are in this PR/change. The main thing to note is that in the past I've allowed some PRs to be merged when they didn't originally actually have tests to assert the behaviour and so I've inadvertently allowed the standards to drop in that regard. Instead I've added those tests to the PR's myself or done a followup PR that adds the tests - we should not do this going forward. That is, any new feature or bug fix or change should include tests that actually exercise that feature. It is not sufficient for something to compile, we actually need tests to assert against the expected behaviour. In this case those tests were missing from this PR. In adding those tests we would have hit the issues with this change (around handling null, and that the tests had invalid URLs. Cheers, Rob. |
- Adds tests that were missing in HelloControllerTest that actually exercises the feature
- Fixes the controller URLs removing incorrect {myOptional} from urls
- Fixes PathTypeConversion to handle null values
- Adds missing javadoc on new public methods of PathTypeConversion
- Adds some useful unit tests for TypeMap.OptionalHandler
I wanted to contribute and asked @SentryMan for a good starter feature. I tried to emulate the style.