Yes you are right. >you must squash the commits. >the commit messages are uninformative
Usually I'd do them when commiter think this pr is ok, and then tell me we should merge it, and at that moment I squash it, and it get merged. the commit messages will be fixed at that time too. >partly revert a previous change Yes, as my newest performance test shows something is not quite right in my previous changes. It makes the codes even slower. So I changed that part. >benchmark are indeed necessary agreed, I did a benchmark yesterday, and pointed a link on github pages. I'll copy/paste >The PR contains different types of changes: one is "use arraycopy" but the others are not documented anywhere (commit message, JIRA). Ah, I forgot to create a jira ticket for it. sorry about that. So, in conclusion, I will close this pr, and make two jira tickets, for the two different types of refines. Thanks for your help. Gilles Sadowski <gillese...@gmail.com> 于2020年6月6日周六 下午7:33写道： > Hi. > > 2020-06-06 10:18 UTC+02:00, GitBox <g...@apache.org>: > > > > XenoAmess opened a new pull request #143: > > URL: https://github.com/apache/commons-math/pull/143 > > > > > > use System.arraycopy instead of loop. > > > > There are several issues with this PR. > > It contains several commits, among which some seem, > IIUC, to partly revert a previous change in that same PR. > It that situation, you must squash the commits (so that > the net changes stand out). > Also, the commit messages are uninformative ("refine", > "fix false positive"). > The PR contains different types of changes: one is "use > arraycopy" but the others are not documented anywhere > (commit message, JIRA). > Traceability and easy reviewing are requirements for > "Commons" components. > For example, modifying the code that fills an array is > not a "minor" change, even if just because it makes the > reviewer wonder "Why?" (and the answer is nowhere to > be found from the official tracking tools). > If some performance improvement is unexpected from > looking at the code change, benchmark are indeed > necessary especially if the new code is less clear (and > a summary of the results should certainly make it into > the commit message and/or the corresponding JIRA > report). [Also, an appropriately named utility method > is probably better than inline statements.] > > Regards, > Gilles > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
