Hi Antonio,
Overall I do agree. The "optimal" PR size however very much depends on
what the PR actually does and how large the changeset is. Even trival
changes when applied to a large enough codebase can cause trouble. Some
tests actually abuse the identity of 'Integer' for example since they
were written before autoboxing existed (I only know that since someone
actually tried that exact refactoring before).
Another example are generics, which is super tiresome to review - this
has to be split up.
Some cleanups can be probably done codebase-wide - I have done them
before (depends on how many files they touch), others should be better
split up.
Thats why I would very much like that the contributors would actually
talk to us instead of front loading 10 PRs ;)
best regards,
michael
On 27.01.23 07:46, Antonio wrote:
Hi,
I'd try to take a look at them during the weekend, as time permits.
I've seen tons of "code cleanup" PRs that have very little value and
are too granular.
- Changing "new Integer(0)" with "0" in three files in a project with
> 500k LOC is not worth the effort and does not provide real value.
- Changing "new String("Hello")" with "Hello" makes sense only if
you're allocating tons of Strings (in a loop, for instance), but not
in a method that you call each ten minutes.
On the contrary, these PRs consume reviewers' time and burn trees
running our 3-flavoured JDK CI/CD pipelines for very little value. Not
worth running those heavy pipelines for just three syntactic sugar
issues in three files.
I'd ask these people concerned with "syntactic sugar" issues to group
those little commits in a single PR, that can be reviewed at once and
that is worth running a single CI/CD run.
Cheers,
Antonio
P.S.: We may also want to define what "readability" means for us.
There're people modifying test code because they think that static
imports are more readable than normal imports. In my book (doing Java
for more than twenty years) I don't see any advantage in using static
imports, on the contrary, you have to scroll up and see what is static
imported.
On 26/1/23 18:59, Michael Bien wrote:
Everyone is invited to review them for correctness and also whether
or not they are actually worth it. Just because there is an easy to
use code inspection available for something doesn't mean we should
run it on 400+ projects blindly. This is also part of the problem:
Running code inspections is trivial, reviewing them however is time
consuming.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@netbeans.apache.org
For additional commands, e-mail: dev-h...@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@netbeans.apache.org
For additional commands, e-mail: dev-h...@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
