You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@netbeans.apache.org by Michael Bien <mb...@gmail.com> on 2023/01/26 17:59:49 UTC
[DISCUSSION] integrating cleanup PRs
Hello devs,
we have a large amount of cleanup PRs open in various stages of
readiness (some are not even labeled yet so the wrong tests ran most
likely).
https://github.com/apache/netbeans/pulls?q=is%3Apr+is%3Aopen+label%3A%22Code+cleanup%22
I stopped integrating cleanup PRs when NB 17 got branched and the
release candidate phase began, since they could potentially cause
unnecessary extra steps for the release team when PRs stop being
portable between master and the delivery branch. I did also want to
avoid merge conflicts with larger last-minute PRs (esp the Jakarta and
PHP one which both made it into NB17).
But since so many PRs piled up by now, I would like to wait till NB
17rc3 is out, and then after that slowly start merging again. Plan is to
let some time pass between merges so that everyone gets the opportunity
to update the local clones and PRs incrementally which should reduce the
risk of conflicts a bit.
would this be ok or should we wait till NB 17 is out?
also:
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.
further:
Best practices for java 5 might no longer be best practices for java 21.
Simple String concatenation with '+' will be really hard to beat
performance wise and readability wise in 95% of all cases. If you find
the 5% - excellent, but provide a benchmark to proof that it makes a
difference for the IDE and it is worth the readability hit. JMH would be
the standard tool for micro benchmarks - it is also used by OpenJDK
itself and fairly easy to set up with maven.
The last point I want to make is that PRs consume CI resources. A CI
run, dependent on what tests are active, takes 4-10h of CI time (we got
better at that since it used to be much more!). So even if you are
willing to open 20 PRs and rebase them again in two weeks you just
wasted up to 20x10h on CI time which could have gone to some other
apache project instead. This has to be done in a smart way without
disturbing the project.
Which is also why I would recommend to wait with opening more cleanup
PRs until the backlog shrinks.
Nobody wants to prevent anybody from working on something, just wait
with the create PR button press.
best regards,
michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSSION] integrating cleanup PRs
Posted by Michael Bien <mb...@gmail.com>.
Hi Antonio,
On 29.01.23 09:26, Antonio wrote:
> Hi Michael,
>
> Refactoring the code because of aesthetic, academic or modernization
> reasons is a non-ending task in a >500KLOC codebase that will turn 27
> this year. By the time we end up modernizing all the code it will then
> be outdated.
Yes, project wide refactoring should be discussed, I am also not willing
to merge any stylistic or no-benefit change sets myself, since its just
noise and disturbs scenarios where git blame is used to reconstruct code
history. A PR needs to have a clear purpose and benefit - not all will
be merged.
Luckily, the amount of project wide inspections a dusty project *should*
run is finite. One example for it is having @Override used across the
codebase - this really is a low hanging fruit - the only reason I
haven't run this refactoring myself yet is because i wanted to write a
hint which ignores compact lambda candidates to shrink the change set.
There are also some "wrong-method" situations which can be fixed this
way, for example:
https://github.com/apache/netbeans/pull/3724
This is not only measurably faster and safer, this even fixed a small
bug (possibly more) - although not trivial to review since you have to
check every case - which I did.
>
> IMHO we need contributors that are conscious of the implications of
> the changes they propose, that run this extra mile that requires
> thinking and explaining us the pros and cons, and the value they want
> to add to the project.
agreed.
>
> IMHO it's our responsibility to request these explanations to
> contributors, otherwise we'll end up stuck in a never-ending loop of
> reviewing a codebase that will turn 27 this year. These discussions,
> as you say, should be done in the mailing list, and not in a PR
> _after_ the code has been submitted.
agreed. Although as mentioned above, I believe the code inspections old
code bases should run are in fact finite. Projects like OpenJDK do run
them too periodically.
best regards,
-mbien
>
> Cheers,
> Antonio
>
>
> On 27/1/23 17:42, Michael Bien wrote:
> [...]
>>
>> Another example are generics, which is super tiresome to review -
>> this has to be split up.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@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-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSSION] integrating cleanup PRs
Posted by Antonio <an...@vieiro.net.INVALID>.
Hi Michael,
Refactoring the code because of aesthetic, academic or modernization
reasons is a non-ending task in a >500KLOC codebase that will turn 27
this year. By the time we end up modernizing all the code it will then
be outdated.
IMHO we need contributors that are conscious of the implications of the
changes they propose, that run this extra mile that requires thinking
and explaining us the pros and cons, and the value they want to add to
the project.
IMHO it's our responsibility to request these explanations to
contributors, otherwise we'll end up stuck in a never-ending loop of
reviewing a codebase that will turn 27 this year. These discussions, as
you say, should be done in the mailing list, and not in a PR _after_ the
code has been submitted.
Cheers,
Antonio
On 27/1/23 17:42, Michael Bien wrote:
[...]
>
> Another example are generics, which is super tiresome to review - this
> has to be split up.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSSION] integrating cleanup PRs
Posted by Michael Bien <mb...@gmail.com>.
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-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@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-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSSION] integrating cleanup PRs
Posted by Antonio <an...@vieiro.net.INVALID>.
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-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSSION] integrating cleanup PRs
Posted by Michael Bien <mb...@gmail.com>.
On 27.01.23 18:04, Michael Bien wrote:
> On 26.01.23 19:18, Neil C Smith wrote:
>> On Thu, 26 Jan 2023 at 18:04, Michael Bien <mb...@gmail.com> wrote:
>>> But since so many PRs piled up by now, I would like to wait till NB
>>> 17rc3 is out, and then after that slowly start merging again. Plan
>>> is to
>>> let some time pass between merges so that everyone gets the opportunity
>>> to update the local clones and PRs incrementally which should reduce
>>> the
>>> risk of conflicts a bit.
>>>
>>> would this be ok or should we wait till NB 17 is out?
>> Well, with releases hat on, I'll repeat what I said before. Feel free
>> to merge but be aware of what's coming into delivery.
>>
>> Or in other words, if you take responsibility for merging something
>> that causes conflicts on sync, you're first on the list to sort out
>> the git mess*! :-)
>
> yes but this is always the case. Merging a PR you haven't written
> yourself is like accepting a side quest :)
speaking of which, planning to merge this one after rc3 unless someone
disagrees:
https://github.com/apache/netbeans/pull/5189
since it is limited to the platform mods, it should have the lowest risk
of causing conflicts during the late-release phase
-mbien
>
> -mbien
>
>>
>> * incidentally be aware that the helpful GitHub UI will break things
>> here.
>>
>>> just wait
>>> with the create PR button press.
>> And please at least do a local compile and run to check it actually
>> manages those things first!
>>
>> Best wishes,
>>
>> Neil
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
>> For additional commands, e-mail: dev-help@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-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSSION] integrating cleanup PRs
Posted by Michael Bien <mb...@gmail.com>.
On 26.01.23 19:18, Neil C Smith wrote:
> On Thu, 26 Jan 2023 at 18:04, Michael Bien <mb...@gmail.com> wrote:
>> But since so many PRs piled up by now, I would like to wait till NB
>> 17rc3 is out, and then after that slowly start merging again. Plan is to
>> let some time pass between merges so that everyone gets the opportunity
>> to update the local clones and PRs incrementally which should reduce the
>> risk of conflicts a bit.
>>
>> would this be ok or should we wait till NB 17 is out?
> Well, with releases hat on, I'll repeat what I said before. Feel free
> to merge but be aware of what's coming into delivery.
>
> Or in other words, if you take responsibility for merging something
> that causes conflicts on sync, you're first on the list to sort out
> the git mess*! :-)
yes but this is always the case. Merging a PR you haven't written
yourself is like accepting a side quest :)
-mbien
>
> * incidentally be aware that the helpful GitHub UI will break things here.
>
>> just wait
>> with the create PR button press.
> And please at least do a local compile and run to check it actually
> manages those things first!
>
> Best wishes,
>
> Neil
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@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-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSSION] integrating cleanup PRs
Posted by Neil C Smith <ne...@apache.org>.
On Thu, 26 Jan 2023 at 18:04, Michael Bien <mb...@gmail.com> wrote:
> But since so many PRs piled up by now, I would like to wait till NB
> 17rc3 is out, and then after that slowly start merging again. Plan is to
> let some time pass between merges so that everyone gets the opportunity
> to update the local clones and PRs incrementally which should reduce the
> risk of conflicts a bit.
>
> would this be ok or should we wait till NB 17 is out?
Well, with releases hat on, I'll repeat what I said before. Feel free
to merge but be aware of what's coming into delivery.
Or in other words, if you take responsibility for merging something
that causes conflicts on sync, you're first on the list to sort out
the git mess*! :-)
* incidentally be aware that the helpful GitHub UI will break things here.
> just wait
> with the create PR button press.
And please at least do a local compile and run to check it actually
manages those things first!
Best wishes,
Neil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists