Guys, The problem with in-place refactorings is that you increase affected scope. It is not uncommon to break compatibility or public contracts with even minor things. E.g. recently we decided drop org.jsr166 package in favor of Java 8 classes. Innocent change. Result - broken storage. Another problem is conflicts. It is not uncommon to have long-lived branches which we need to merge with master over and over again. And a lot of refactorings cause conflicts. It is much easier to resolve them if you know that logic was not affected as opposed to cases when you need to resolve both renames and method extractions along with business-logic changes.
I'd like to repeat - if you have a time for refactoring then you definitely have a time to extract these changes to separate PR and submit a separate ticket. I am quite understand what "low priority" do you mean if you do refactorings on your own. On Tue, Apr 24, 2018 at 10:52 PM, Andrey Kuznetsov <stku...@gmail.com> wrote: > +1. > > Once again, I beg for "small refactoring permission" in a checklist. As of > today, separate tickets for small refactorings has lowest priority, since > they neither fix any flaw nor add new functionality. Also, the attempts to > make issue-related code safer / cleaner / more readable in "real" pull > requests are typically rejected, since they contradict our current > guidelines. > > I understand this will require a bit more effort from committer/maintainer, > but otherwise we will get constantly degrading code quality. > > > 2018-04-24 18:52 GMT+03:00 Eduard Shangareev <eduard.shangar...@gmail.com > >: > > > Vladimir, > > > > I am not talking about massive/sophisticated refactoring. But I believe > > that ask to extract some methods should be OK to do without an extra > > ticket. > > > > A checklist shouldn't be necessarily a set of certain rules but also it > > could include suggestion and reminders. > > > > On Tue, Apr 24, 2018 at 6:39 PM, Vladimir Ozerov <voze...@gridgain.com> > > wrote: > > > > > Ed, > > > > > > Refactoring is a separate task. If you would like to rework exchange > > future > > > - please do this in a ticket "Refactor exchange task", nobody would > > against > > > this. This is just a matter of creating separate ticket and separate > PR. > > If > > > one have a time for refactoring, it should not be a problem for him to > > > spend several minutes on JIRA and GitHub. > > > > > > As far as documentation - what you describe is normal review process, > > when > > > reviewer might want to ask contributor to fix something. Checklist is a > > > different thing - this is a set of rules which must be followed by > > anyone. > > > I do not understand how you can define documentation in this checklist. > > > Same problem with logging - what is "enough"? > > > > > > On Tue, Apr 24, 2018 at 4:51 PM, Eduard Shangareev < > > > eduard.shangar...@gmail.com> wrote: > > > > > > > Igniters, > > > > > > > > I don't understand why you are so against refactoring. > > > > Code already smells like hell. Methods 200+ line is normal. Exchange > > > future > > > > is asking to be separated on several one. Transaction code could > > > understand > > > > few people. > > > > > > > > If we separate refactoring from development it would mean that no one > > > will > > > > do it. > > > > > > > > > > > > 2) Documentation. > > > > Everything which was asked by reviewers to clarify idea should be > > > reflected > > > > in the code. > > > > > > > > 3) Logging. > > > > Logging should be enough to troubleshoot the problem if someone comes > > to > > > > user-list with an issue in the code. > > > > > > > > > > > > On Fri, Apr 20, 2018 at 7:06 PM, Dmitry Pavlov < > dpavlov....@gmail.com> > > > > wrote: > > > > > > > > > Hi Igniters, > > > > > > > > > > +1 to idea of checklist. > > > > > > > > > > +1 to refactoring and documenting code related to ticket in +/-20 > LOC > > > at > > > > > least. > > > > > > > > > > If we start to do it as part of our regular contribution, code will > > be > > > > > better, it would became common practice and part of Apache Ignite > > > > > development culure. > > > > > > > > > > If we will hope we will have free time to submit separate patch > > someday > > > > and > > > > > have patience to complete patch-submission process, code will > remain > > > > > undocumented and poor-readable. > > > > > > > > > > Sincerely, > > > > > Dmitriy Pavlov > > > > > > > > > > пт, 20 апр. 2018 г. в 18:56, Александр Меньшиков < > > sharple...@gmail.com > > > >: > > > > > > > > > > > 4) Metrics. > > > > > > partially +1 > > > > > > > > > > > > It makes sense to have some minimal code coverage for new code in > > PR. > > > > > IMHO. > > > > > > > > > > > > Also, we can limit the cyclomatic complexity of the new code in > PR > > > too. > > > > > > > > > > > > 6) Refactoring > > > > > > -1 > > > > > > > > > > > > I understand why people want to refactor old code. > > > > > > But I think refactoring should be always a separate task. > > > > > > And it's better to remove all refactoring from PR, if it's not > the > > > > sense > > > > > of > > > > > > the issue. > > > > > > > > > > > > > > > > > > 2018-04-20 16:54 GMT+03:00 Andrey Kuznetsov <stku...@gmail.com>: > > > > > > > > > > > > > What about adding the following item to the checklist: when the > > > > change > > > > > > adds > > > > > > > new functionality, then unit tests should also be provided, if > > it's > > > > > > > technically possible? > > > > > > > > > > > > > > As for refactorings, in fact they are strongly discouraged > today > > > for > > > > > some > > > > > > > unclear reason. Let's permit to make refactorings in the > > checklist > > > > > being > > > > > > > discussed. (Of cource, refactoring should relate to problem > being > > > > > > solved.) > > > > > > > > > > > > > > 2018-04-20 16:16 GMT+03:00 Vladimir Ozerov < > voze...@gridgain.com > > >: > > > > > > > > > > > > > > > Hi Ed, > > > > > > > > > > > > > > > > Unfortunately some of these points are not good candidates > for > > > the > > > > > > > > checklist because of these: > > > > > > > > - It must be clear and disallow *multiple interpretations* > > > > > > > > - It must be *lightweight*, otherwise Ignite development > would > > > > > become a > > > > > > > > nightmare > > > > > > > > > > > > > > > > We cannot have "nice to have" points here. Checklist should > > > answer > > > > > the > > > > > > > > question "is ticket eligible to be merged?" > > > > > > > > > > > > > > > > >>> 1) Code style. > > > > > > > > +1 > > > > > > > > > > > > > > > > >>> 2) Documentation > > > > > > > > -1, it is impossible to define what is "well-documented". A > > piece > > > > of > > > > > > code > > > > > > > > could be obvious for one contributor, and non-obvious for > > > another. > > > > In > > > > > > any > > > > > > > > case this is not a blocker for merge. Instead, during review > > one > > > > can > > > > > > ask > > > > > > > > implementer to add more docs, but it cannot be forced. > > > > > > > > > > > > > > > > >>> 3) Logging > > > > > > > > -1, same problem - what is "enough logging?". Enough for > whom? > > > How > > > > to > > > > > > > > understand whether it is enough or not? > > > > > > > > > > > > > > > > >>> 4) Metrics > > > > > > > > -1, no clear boundaries, and decision on whether metrics are > to > > > be > > > > > > added > > > > > > > or > > > > > > > > not should be performed during design phase. As before, it is > > > > > perfectly > > > > > > > > valid to ask contributor to add metrics with clear > explanation > > > why, > > > > > but > > > > > > > > this is not part of the checklist. > > > > > > > > > > > > > > > > >>> 5) TC status > > > > > > > > +1, already mentioned > > > > > > > > > > > > > > > > >>> 6) Refactoring > > > > > > > > Strong -1. OOP is a slippery slope, there are no good and bad > > > > > receipts > > > > > > > for > > > > > > > > all cases, hence it cannot be used in a checklist. > > > > > > > > > > > > > > > > We can borrow useful rules from p.2, p.3 and p.4 if you > provide > > > > clear > > > > > > > > definitions on how to measure them. > > > > > > > > > > > > > > > > Vladimir. > > > > > > > > > > > > > > > > On Fri, Apr 20, 2018 at 3:50 PM, Eduard Shangareev < > > > > > > > > eduard.shangar...@gmail.com> wrote: > > > > > > > > > > > > > > > > > Also, I want to add some technical requirement. Let's > discuss > > > > them. > > > > > > > > > > > > > > > > > > 1) Code style. > > > > > > > > > The code needs to be formatted according to coding > guidelines > > > > > > > > > < > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/ > > Coding+Guidelines > > > > > > > >. > > > > > > > > > The > > > > > > > > > code must not contain TODOs without a ticket reference. > > > > > > > > > > > > > > > > > > It is highly recommended to make major formatting changes > in > > > > > existing > > > > > > > > code > > > > > > > > > as a separate commit, to make review process more > practical. > > > > > > > > > > > > > > > > > > 2) Documentation. > > > > > > > > > Added code should be well-documented. Any methods that > raise > > > > > > questions > > > > > > > > > regarding their code flow, invariants, synchronization, > etc., > > > > must > > > > > be > > > > > > > > > documented with comprehensive javadoc. Any reviewer can > > request > > > > > that > > > > > > a > > > > > > > > > particular added method be documented. Also, it is a good > > > > practice > > > > > to > > > > > > > > > document old code in a 10-20 lines region around changed > > code. > > > > > > > > > > > > > > > > > > 3) Logging. > > > > > > > > > Make sure that there are enough logging added in every > > category > > > > for > > > > > > > > > possible diagnostic in field. Check that logging messages > are > > > > > > properly > > > > > > > > > spelled. > > > > > > > > > > > > > > > > > > 4) Metrics. > > > > > > > > > Are there any metrics that need to be exposed to user? > > > > > > > > > > > > > > > > > > 5) TC status. > > > > > > > > > > > > > > > > > > Recheck that there are no new failing tests > > > > > > > > > > > > > > > > > > 6) Refactoring. > > > > > > > > > > > > > > > > > > The code should be better than before: > > > > > > > > > > > > > > > > > > - extract method from big one; > > > > > > > > > - do anything else to make code clearer (don't forget > > about > > > > some > > > > > > > > > OOP-practise, replace if-else hell with inheritance > > > > > > > > > - split refactoring (renaming, code format) from actual > > > > changes > > > > > by > > > > > > > > > separate commit > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev < > > > > > > > > > eduard.shangar...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > Hi, guys. > > > > > > > > > > > > > > > > > > > > I believe that we should update maintainers list before > > > adding > > > > > this > > > > > > > > > review > > > > > > > > > > requirement. > > > > > > > > > > There should not be the situation when there is only one > > > > > > contributor > > > > > > > > who > > > > > > > > > > is responsible for a component. > > > > > > > > > > We already have issues with review speed and response > time. > > > > > > > > > > > > > > > > > > > > On Fri, Apr 20, 2018 at 2:17 PM, Anton Vinogradov < > > > > a...@apache.org > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > >> Vova, > > > > > > > > > >> > > > > > > > > > >> Everything you described sound good to me. > > > > > > > > > >> > > > > > > > > > >> I'd like to propose to create special page at AI Wiki > and > > to > > > > > > > describe > > > > > > > > > >> checklist. > > > > > > > > > >> In case we'll find something should be changed/improved > it > > > > will > > > > > be > > > > > > > > easy > > > > > > > > > to > > > > > > > > > >> update the page. > > > > > > > > > >> > > > > > > > > > >> 2018-04-20 0:53 GMT+03:00 Nikolay Izhikov < > > > > nizhi...@apache.org > > > > > >: > > > > > > > > > >> > > > > > > > > > >> > Hello, Vladimir. > > > > > > > > > >> > > > > > > > > > > >> > Thank you for seting up this discussion. > > > > > > > > > >> > > > > > > > > > > >> > As we discussed, I think an important part of this > check > > > > list > > > > > is > > > > > > > > > >> > compatibility rules. > > > > > > > > > >> > > > > > > > > > > >> > * What should be backward compatible? > > > > > > > > > >> > * How should we maintain it? > > > > > > > > > >> > > > > > > > > > > >> > > 3) If ticket changes public API or existing > behavior, > > at > > > > > least > > > > > > > two > > > > > > > > > >> > commiters should approve the changes > > > > > > > > > >> > > > > > > > > > > >> > We can learn from other open source project > experience. > > > > > > > > > >> > Apache Kafka [1], for example, requires KIP(kafka > > > > improvement > > > > > > > > > proposal) > > > > > > > > > >> > for *every* major change. > > > > > > > > > >> > Major change definition includes public API. > > > > > > > > > >> > > > > > > > > > > >> > [1] https://cwiki.apache.org/ > confluence/display/KAFKA/ > > > > > > > > > >> > Kafka+Improvement+Proposals > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > В Чт, 19/04/2018 в 23:00 +0300, Vladimir Ozerov пишет: > > > > > > > > > >> > > Hi Igniters, > > > > > > > > > >> > > > > > > > > > > > >> > > It's glad to see our community becomes larger every > > day. > > > > But > > > > > > as > > > > > > > it > > > > > > > > > >> grows > > > > > > > > > >> > it > > > > > > > > > >> > > becomes more and more difficult to manage review and > > > merge > > > > > > > > processes > > > > > > > > > >> and > > > > > > > > > >> > > keep quality of our decisions at the proper level. > > More > > > > > > > > > contributors, > > > > > > > > > >> > more > > > > > > > > > >> > > commits, more components interlinked with each other > > in > > > > > subtle > > > > > > > > ways. > > > > > > > > > >> > > > > > > > > > > > >> > > I would like to propose to setup a formal review > > > > checklist. > > > > > > This > > > > > > > > > would > > > > > > > > > >> > be a > > > > > > > > > >> > > set of actions every reviewer needs to check before > > > > > approving > > > > > > > > merge > > > > > > > > > >> of a > > > > > > > > > >> > > certain feature. Passing the checklist would be > > > *necessary > > > > > but > > > > > > > not > > > > > > > > > >> > > sufficient* phase before commit could be added to > the > > > main > > > > > > > branch. > > > > > > > > > The > > > > > > > > > >> > > checklist would help us to detect a lot of common > > > problems > > > > > > such > > > > > > > a > > > > > > > > > >> broken > > > > > > > > > >> > > tests or bad UX earlier, and would help contributors > > > lead > > > > > > their > > > > > > > > pull > > > > > > > > > >> > > requests to merge. > > > > > > > > > >> > > > > > > > > > > > >> > > Hallmarks of a good checklist: > > > > > > > > > >> > > - It must be followed be everyone without exceptions > > > > > > > > > >> > > - It must be clear and disallow multiple > > interpretations > > > > > > > > > >> > > - It must be lightweight, otherwise Ignite > development > > > > would > > > > > > > > become > > > > > > > > > a > > > > > > > > > >> > > nightmare > > > > > > > > > >> > > - It must be non-blocking, i.e. inacessibility of a > > > single > > > > > > > > > contributor > > > > > > > > > >> > > should not block ticket progress forever. > > > > > > > > > >> > > > > > > > > > > > >> > > Please let me know if you think the idea makes > sense. > > If > > > > we > > > > > > > agree > > > > > > > > on > > > > > > > > > >> it, > > > > > > > > > >> > > let's start defining action items for the checklist. > > My > > > 2 > > > > > > cents: > > > > > > > > > >> > > 1) All unit tests pass on TC without new failures > > > > > > > > > >> > > 2) If ticket targets specific component, it should > be > > > > > reviewed > > > > > > > by > > > > > > > > > >> > > component's maintainer* > > > > > > > > > >> > > 3) If ticket changes public API or existing > behavior, > > at > > > > > least > > > > > > > two > > > > > > > > > >> > > commiters should approve the changes ** > > > > > > > > > >> > > > > > > > > > > > >> > > Thoughts? > > > > > > > > > >> > > > > > > > > > > > >> > > Vladimir. > > > > > > > > > >> > > > > > > > > > > > >> > > * TBD: Review component list and define maintainers; > > > > define > > > > > > what > > > > > > > > to > > > > > > > > > >> do if > > > > > > > > > >> > > maintainer is unavailable > > > > > > > > > >> > > ** TBD: Define what is "public API" > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > Best regards, > > > > > > > Andrey Kuznetsov. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > Best regards, > Andrey Kuznetsov. >
