2020-07-26 17:34 UTC+02:00, Xeno Amess <xenoam...@gmail.com>: > Thanks for the suggestions about JIRA message and commit style. > Will find some time to refine the texts. > >> ... randomly picking LANG-1576 (sorry if the others don't fit the >> following), I'll stress again that there are more important things >> to do before such (supposed) performance enhancement. >> These are changes which a committer might do, but that are not >> worth a reviewer's time unless it comes with benchmarks that >> prove the claim (see for example the work done by Alex to >> squeeze out the last drops of performance in "Commons RNG"). > > I do not quite agree, as this is a base library, not a software. > If it be a software, yes, unless we meet bottle-neck we should not > over-optimize. > But this is a base library, so IMO we should squeeze out as much > performance as we can, everywhere, because we actually cannot make sure > which function could be widely used by who.
I specifically commented on LANG-1576. Where is the benchmark? Gilles > >> For example, following Gary's and Bruno's comments, you >> could set up a branch that would delete all the deprecated >> codes > > I think that should be done just before we make lang 4.0. > But as far as I know, lang 4.0 is still far away, at least several months > time later, so I don't think this is the right time to take action to > remove the deprecated codes... > Means maintaining such a branch for several months seems not quite worthy. > >> and look for further code bloat that could be removed from the next major > release > > I will if I see any. > > > Gilles Sadowski <gillese...@gmail.com> 于2020年7月26日周日 下午11:13写道： > >> Hi Xeno. >> >> 2020-07-26 13:10 UTC+02:00, Xeno Amess <xenoam...@gmail.com>: >> >>> For examples about my prs at commons-lang,if my memory is correct, >> >>> only >> >>> gary (and sometimes kinow) reviewed my prs, and I don't think we have >> > only >> >>> two committers in commons-lang. >> > >> >> Are there JIRA reports? >> > >> > My log here is: >> > >> > LANG-1545 merged by gary >> > LANG-1561 merged by gary >> > LANG-1563 merged by gary >> > LANG-1562 merged by gary >> > LANG-1564 merged by gary >> > LANG-1560 merged by gary >> > LANG-1552 merged by gary >> > LANG-1553 merged by gary >> > LANG-1554 merged by gary >> > LANG-1555 merged by gary >> > LANG-1558 merged by gary >> > LANG-1559 merged by gary >> > LANG-1556 merged by gary >> > LANG-1565 merged by gary >> > LANG-1557 merged by gary >> > LANG-1546 merged by kinow >> > LANG-1549 merged by chtompki >> >> As said, thanks for your interest in "Commons" but... >> >> > pending: >> > 10+ sub-quests in LANG-1573 pending, for near 1 month already. most of >> > which is performance refines in StringUtils. >> >> ... randomly picking LANG-1576 (sorry if the others don't fit the >> following), I'll stress again that there are more important things >> to do before such (supposed) performance enhancement. >> These are changes which a committer might do, but that are not >> worth a reviewer's time unless it comes with benchmarks that >> prove the claim (see for example the work done by Alex to >> squeeze out the last drops of performance in "Commons RNG"). >> >> Also, please be more informative in the title of the reports: >> "refine <something>" says that you changed <something> >> but not how or why. Then by going to the JIRA report itself, we >> don't get more information; the "description" field should >> contain a description, not just the link to the PR. >> So a potential reviewer, instead of getting a direct hint about >> whether he could have the knowledge or interest in committing >> the changes, must go all the way (link in the notification mail, >> link in JIRA, often multiple links in the PR's GH page) to the diff >> itself, to figure out that in LANG-1576, you changed "if ... else" >> statements to a "switch" statement. >> There are indeed 2 commits there (instead of 1 as I've stressed >> several times already) and that just increases the review time >> because: >> * I click on the last one and I don't see the diff with "master" >> but only the diff wrt your changes. >> * Then I click on the first commit and and wonder: Is it OK to >> have a fall-through there? >> * Then I go back to the list of commit and wonder: What was >> the CheckStyle issue? >> * Then I click again the last commit and see that there is now >> a duplicate a statement and wonder: Is that necessary[2], or >> is it cutting corners to prevent the CheckStyle warning and >> let Travis green-light the change? >> * Then I figure out that I cannot take the responsibility to >> make the commit because the improvement is not obvious. >> >> > I'm not requesting somebody must review my pr now or something. >> > And I know committers are busy. >> > I say this just for showing, in my view, we really have no enough >> > reviewers. >> > And if somebody has any ideas about how we can solve this, by making >> > more >> > reviewers or making current non-active committers more active, or other >> > more advice... >> >> You can help the project by taking on the suggestion which >> I've already made, and that amounts to increasing the ratio >> of contribution time to review time. >> For example, following Gary's and Bruno's comments, you >> could set up a branch that would delete all the deprecated >> codes, and look for further code bloat that could be removed >> from the next major release, and ensure that alternatives are >> working and advertised in the Javadoc and release notes. >> >> Regards, >> Gilles >> >> [1] https://issues.apache.org/jira/browse/LANG-1576 >> [2] >> https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.html >> >> >> [...] >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
