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. >
