michaelkarnerfors commented on PR #458: URL: https://github.com/apache/commons-text/pull/458#issuecomment-1721966419
> -1 as is: > > * This PR will break the build: Run `mvn` to run the default Maven goal which in turn runs all build checks > * Don't rewrite existing tests: It makes it harder to review the PR and adds the possibility of changing test/feature coverage. It's better to add new test methods or add to existing tests methods. > * Don't clutter the tests with @DisplayName, use Javadoc if a test needs explanations, this provides better formatting. > * In general, keep changes to a minimum to easy the reviews. * After numerous attempts at building both master and this PR with 'mvn', I find no difference between the results. One test (DoubleFormatTest.testPlain_localeFormatComparison) that is unrelated to the change is broken in both master and this PR. Also another text breaks unless I set my machine to en_US, but — again — this happens in both master and this PR, and is unrelated. If you can point to any tests that fail at your end, I will happily take a look at them. * _"It makes it harder to review the PR"_ Yes, improvements do increase the task of reviewing a PR. If it is the case — that keeping the workload of reviewing a PR to an abject minimum — is a vetoing priority in Apache Commons Text — one that prevents anyone from doing things such improving existing code — then let me know and I will revert back to the old format. If not however, I consider the (in my not very humble opinion) messy tests, with uninformative names, that have not been touched up in up to 7 years, to be debt that should be dealt with and modernised. * _"the possibility of changing test/feature coverage"_ Since new functionality was added, the test coverage increased, yes. All the old tests has remained organised as they were before, and the contents are essentially untouched, with the exception for test cases that accepted broken behaviour. * _"add new test methods or add to existing tests methods"_ I did just that, too. * Javadoc does not provide the functionality of @DisplayName and is wholly unsuitable for the intended purpose for which the annotations were added. To explain the purpose of using @DisplayName and parametrised tests, here is a comparative screenshot from Eclipse.  _Unannotated test cases to the left, annotated on the right_ IntelliJ IDEA also supports @DisplayName and parametrised tests (and [Jetbrains even recommends it]( https://www.jetbrains.com/idea/guide/tutorials/writing-junit5-tests/helpful-test-names-for-display/)), as does Visual Studio Code.  Hence the comment is counterproductive: Javadoc adds _more_ "clutter" than @DisplayName, _without_ achieving the purpose for which the annotation was added. * _"In general, keep changes to a minimum to easy the reviews"_ I agree with this sentiment. With this in mind, lessening debt, increasing maintainability and aiding future development, is well worth one more PR in 7 years, I think you will agree. The very purpose of Open Source / Free Software is — after all — that they shall(!) be able to be improved over time. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
