strangelookingnerd commented on PR #324:
URL: https://github.com/apache/commons-dbutils/pull/324#issuecomment-2488640361
> Please be aware that in our post-XZ Utils world of supply chain attacks, a
maintainer MUST review every single line of code, not just eyeball a file. A PR
like one of 45 files and hundreds if not thousands of changes puts a painful
burden on a maintainer like myself.
100% understand.
> There is no need to edit every single test file to change the visibility
of classes of methods.
It's considered best practice for JUnit5 and I find it useful as it for
example excludes those classes and methods from auto-completion and removes the
burden to add Javadoc to them. As most IDEs / code analysis tools require
Javadoc on `public` classes and methods, we often time end up with useless,
verbose information. Or at least I personally do not see the need to add
something like this for every class
```java
/**
* Test the BasicRowProcessor class.
*/
public class BasicRowProcessorTest extends BaseTestCase {
```
```java
/**
* ColumnListHandlerTest
*/
public class ColumnListHandlerTest extends BaseTestCase {
```
My proposal would be to follow the best practice of reducing the visibility
of test classes and methods and remove superfluous Javadoc as well. WDYT?
Totally fine if you disagree, just let me know.
> There is no need to edit JDBC implementations of methods to remove
SQLExceptions from method signatures, this has nothing to do with JUnit 5.
Fair point, I put it under "trivial cleanup" since I was touching these
methods anyway. I'll revert it.
> Review your own work, don't apply search and replace without examining the
results, I think you would have caught the indentation issues.
I am mostly applying OpenRewrite recipes and validate / adapt the results
manually. No clue how the indention got messed up here but I will re-check for
sure. Maybe adding `spotless` to the build would help.
> Run 'mvn' (by itself) to run all build checks (the default Maven goal);
running 'mvn clean verify' is not enough ;-)
In my defence, `CONTRIBUTING.md` actually states to run `mvn clean verify`.
--
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]
