Igniters, I created review checklist on WIKI [1] and also fixed related pages (e.g. "How To Contribute"). Please let me know if you have any comments before I go with public announce.
Vladimir. [1] https://cwiki.apache.org/confluence/display/IGNITE/Review+Checklist On Thu, May 10, 2018 at 5:10 PM, Vladimir Ozerov <voze...@gridgain.com> wrote: > Ilya, > > We define that exception messages *SHOULD* have clear explanation on what > is wrong. *SHOULD* mean that the rule should be followed unless there is a > reason not to follow. In your case you refer to some unexpected behavior. > I.e. an exceptional situation developer is not aware of. In this case for > sure we cannot force contributor to explain what is wrong, because, well, > we don't know. This is why we relaxed the rule from *MUST* to *SHOULD*. > > On Thu, May 10, 2018 at 3:50 PM, Ilya Kasnacheev < > ilya.kasnach...@gmail.com> wrote: > >> I don't think I quite understand how exception explanations should work. >> >> Imagine we have the following exception: >> >> // At least RuntimeException can be thrown by the code above when >> GridCacheContext is cleaned and there is >> // an attempt to use cleaned resources. >> U.error(log, "Unexpected exception during cache update", e); >> >> I mean, we genuinely don't know what happened here. >> >> Under new rules, what kind of "workaround" would that exception suggest? >> "Try turning it off and then back on"? >> What explanation how to resolve this exception can we offer? "Please write >> to d...@apache.ignite.org or to Apache JIRA, and then wait for a release >> with fix?" >> >> I'm really confused how we can implement 1.6 and 1.7 when dealing with >> messy real-world code. >> >> Regards, >> >> >> -- >> Ilya Kasnacheev >> >> 2018-05-10 11:39 GMT+03:00 Vladimir Ozerov <voze...@gridgain.com>: >> >> > Andrey, Anton, Alex >> > >> > Agree, *SHOULD* is more appropriate here. >> > >> > Please see latest version below. Does anyone want to add or change >> > something? Let's wait for several days for more feedback and then >> publish >> > and announce this list. Note that it would not be carved in stone and we >> > will be able to change it at any time if needed. >> > >> > 1) API >> > 1.1) API compatibility *MUST* be maintained between minor releases. Do >> not >> > remove existing methods or change their signatures, deprecate them >> instead >> > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases, >> > unless absolutely needed. If change is made, it *MUST* be described in >> > "Migration Guide" >> > 1.3) New operation *MUST* be well-documented in code (javadoc, >> dotnetdoc): >> > documentation must contain method's purpose, description of parameters >> and >> > how their values affect the outcome, description of return value and >> it's >> > default, behavior in negative cases, interaction with other operations >> and >> > components >> > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained >> when >> > operation makes sense on both platforms. If method cannot be >> implemented in >> > a platform immediately, new JIRA ticket *MUST* be created and linked to >> > current ticket >> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained >> > when operation makes sense on several clients. If method cannot be >> > implemented in a client immediately, new JIRA ticket *MUST* be created >> and >> > linked to current ticket >> > 1.6) All exceptions thrown to a user **SHOULD** have explanation how to >> > resolve, workaround or debug an error >> > >> > 2) Compatibility >> > 2.1) Persistence backward compatibility *MUST* be maintained between >> minor >> > releases. It should be possible to start newer version on data files >> > created by the previous version >> > 2.2) Thin client forward and backward compatibility *SHOULD* be >> maintained >> > between two consecutive minor releases. If compatibility cannot be >> > maintained it *MUST* be described in "Migration Guide" >> > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be >> > maintained between two consecutive minor releases. If compatibility >> cannot >> > be maintained it *MUST* be described in "Migration Guide" >> > >> > 3) Tests >> > 3.1) New functionality *MUST* be covered with unit tests for both >> positive >> > and negative use cases >> > 3.2) All test suites *MUST* be run before merge to master..There *MUST* >> be >> > no new test failures >> > >> > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines >> > >> > Vladimir. >> > >> > >> > On Tue, May 8, 2018 at 1:05 PM, Andrey Kuznetsov <stku...@gmail.com> >> > wrote: >> > >> > > Anton, >> > > >> > > I agree, *MUST* for exception reasons and *SHOULD* for ways of >> resolution >> > > sound clearer. >> > > >> > > 2018-05-08 12:56 GMT+03:00 Anton Vinogradov <a...@apache.org>: >> > > >> > > > Andrey, >> > > > >> > > > How about >> > > > 1.6) All exceptions thrown to a user *MUST* have explanation of >> > > workaround >> > > > and contain original error. >> > > > All exceptions thrown to a user *SHOULD* have explanation how to >> > resolve >> > > if >> > > > possible. >> > > > ? >> > > > >> > > > вт, 8 мая 2018 г. в 12:26, Andrey Kuznetsov <stku...@gmail.com>: >> > > > >> > > > > Vladimir, checklist looks pleasant enough for me. >> > > > > >> > > > > I'd like to suggest one minor change. In 1.6 *MUST* seems to be >> too >> > > > strict, >> > > > > *SHOULD* would be enough. It can be frustrating for API user if I >> > > explain >> > > > > how to fix NPEs in a trivial way, for example. >> > > > > >> > > > > 2018-05-08 11:34 GMT+03:00 Anton Vinogradov <a...@apache.org>: >> > > > > >> > > > > > Alex, >> > > > > > >> > > > > > It is not sounds like that, obviously. >> > > > > > >> > > > > > Tests should cover all negative and positive cases. >> > > > > > You should add enough tests to cover all cases. >> > > > > > >> > > > > > Sometimes one test can cover more than one case, so two tests >> *CAN* >> > > > > > partially check same things. >> > > > > > In case some cases already covered you should not create >> > duplicates. >> > > > > > >> > > > > > вт, 8 мая 2018 г. в 10:19, Александр Меньшиков < >> > sharple...@gmail.com >> > > >: >> > > > > > >> > > > > > > Vladimir, the 3.1 is a bit unclear for me. Which code >> coverage is >> > > > > > > acceptable? Now it sounds like two tests are enough (one for >> > > positive >> > > > > and >> > > > > > > one for negative cases). >> > > > > > > >> > > > > > > 2018-05-07 23:09 GMT+03:00 Dmitriy Setrakyan < >> > > dsetrak...@apache.org >> > > > >: >> > > > > > > >> > > > > > > > Is this list on the Wiki? >> > > > > > > > >> > > > > > > > On Mon, May 7, 2018 at 7:26 AM, Vladimir Ozerov < >> > > > > voze...@gridgain.com> >> > > > > > > > wrote: >> > > > > > > > >> > > > > > > > > Igniters, >> > > > > > > > > >> > > > > > > > > This is the checklist I have at the moment. Please let me >> > know >> > > if >> > > > > you >> > > > > > > > have >> > > > > > > > > any comments on existing items, or want to add or remove >> > > > anything. >> > > > > It >> > > > > > > > looks >> > > > > > > > > like we may have not only strict rules, but *nice to have* >> > > points >> > > > > > here >> > > > > > > as >> > > > > > > > > well with help of *MUST*, *SHOULD* and *MAY* words as per >> > > RFC2119 >> > > > > > [1]. >> > > > > > > So >> > > > > > > > > please feel free to suggest optional items as well. >> > > > > > > > > >> > > > > > > > > 1) API >> > > > > > > > > 1.1) API compatibility *MUST* be maintained between minor >> > > > releases. >> > > > > > Do >> > > > > > > > not >> > > > > > > > > remove existing methods or change their signatures, >> deprecate >> > > > them >> > > > > > > > instead >> > > > > > > > > 1.2) Default behaviour "SHOULD NOT* be changed between >> minor >> > > > > > releases, >> > > > > > > > > unless absolutely needed. If change is made, it *MUST* be >> > > > described >> > > > > > in >> > > > > > > > > "Migration Guide" >> > > > > > > > > 1.3) New operation *MUST* be well-documented in code >> > (javadoc, >> > > > > > > > dotnetdoc): >> > > > > > > > > documentation must contain method's purpose, description >> of >> > > > > > parameters >> > > > > > > > and >> > > > > > > > > how their values affect the outcome, description of return >> > > value >> > > > > and >> > > > > > > it's >> > > > > > > > > default, behavior in negative cases, interaction with >> other >> > > > > > operations >> > > > > > > > and >> > > > > > > > > components >> > > > > > > > > 1.4) API parity between Java and .NET platforms *SHOULD* >> be >> > > > > > maintained >> > > > > > > > when >> > > > > > > > > operation makes sense on both platforms. If method cannot >> be >> > > > > > > implemented >> > > > > > > > in >> > > > > > > > > a platform immediately, new JIRA ticket *MUST* be created >> and >> > > > > linked >> > > > > > to >> > > > > > > > > current ticket >> > > > > > > > > 1.5) API parity between thin clients (Java, .NET) >> *SHOULD* be >> > > > > > > maintained >> > > > > > > > > when operation makes sense on several clients. If method >> > cannot >> > > > be >> > > > > > > > > implemented in a client immediately, new JIRA ticket >> *MUST* >> > be >> > > > > > created >> > > > > > > > and >> > > > > > > > > linked to current ticket >> > > > > > > > > 1.6) All exceptions thrown to a user *MUST* have >> explanation >> > > how >> > > > to >> > > > > > > > > resolve, workaround or debug an error >> > > > > > > > > >> > > > > > > > > 2) Compatibility >> > > > > > > > > 2.1) Persistence backward compatibility *MUST* be >> maintained >> > > > > between >> > > > > > > > minor >> > > > > > > > > releases. It should be possible to start newer version on >> > data >> > > > > files >> > > > > > > > > created by the previous version >> > > > > > > > > 2.2) Thin client forward and backward compatibility >> *SHOULD* >> > be >> > > > > > > > maintained >> > > > > > > > > between two consecutive minor releases. If compatibility >> > cannot >> > > > be >> > > > > > > > > maintained it *MUST* be described in "Migration Guide" >> > > > > > > > > 2.3) JDBC and ODBC forward and backward compatibility >> > *SHOULD* >> > > be >> > > > > > > > > maintained between two consecutive minor releases. If >> > > > compatibility >> > > > > > > > cannot >> > > > > > > > > be maintained it *MUST* be described in "Migration Guide" >> > > > > > > > > >> > > > > > > > > 3) Tests >> > > > > > > > > 3.1) New functionality *MUST* be covered with unit tests >> for >> > > both >> > > > > > > > positive >> > > > > > > > > and negative use cases >> > > > > > > > > 3.2) All test suites *MUST* be run before merge to >> > > master..There >> > > > > > *MUST* >> > > > > > > > be >> > > > > > > > > no new test failures >> > > > > > > > > >> > > > > > > > > 4) Code style *MUST* be followed as per Ignite's Coding >> > > > Guidelines >> > > > > > > > > >> > > > > > > > > Vladimir. >> > > > > > > > > >> > > > > > > > > [1] https://www.ietf.org/rfc/rfc2119.txt >> > > > > > > > > >> > > > > > > > > On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov < >> > > > > > voze...@gridgain.com> >> > > > > > > > > wrote: >> > > > > > > > > >> > > > > > > > > > Hi Dmitry, >> > > > > > > > > > >> > > > > > > > > > Yes, I'll do that in the nearest days. >> > > > > > > > > > >> > > > > > > > > > On Wed, Apr 25, 2018 at 8:24 PM, Dmitry Pavlov < >> > > > > > > dpavlov....@gmail.com> >> > > > > > > > > > wrote: >> > > > > > > > > > >> > > > > > > > > >> Igniters, the idea was related to small refactorings >> > > > co-located >> > > > > > with >> > > > > > > > > main >> > > > > > > > > >> change. >> > > > > > > > > >> >> > > > > > > > > >> Main change itself indicates that existing code did not >> > meet >> > > > the >> > > > > > > > > criteria >> > > > > > > > > >> of practice. Approving of standalone refactorings >> instead >> > > > > > > contradicts >> > > > > > > > > with >> > > > > > > > > >> principle don't touch if it works. So I still like >> idea of >> > > > > > > co-located >> > > > > > > > > >> changes improving code, javadocs, style, etc. >> > > > > > > > > >> >> > > > > > > > > >> But let's not argue about this point now, let's >> summarize >> > > the >> > > > > > > > undisputed >> > > > > > > > > >> points and add it to the wiki. Vladimir, would you >> please >> > do >> > > > it? >> > > > > > > > > >> >> > > > > > > > > >> >> > > > > > > > > >> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov < >> > > > > nizhi...@apache.org >> > > > > > >: >> > > > > > > > > >> >> > > > > > > > > >> > Igniters, >> > > > > > > > > >> > >> > > > > > > > > >> > I agree with Vova. >> > > > > > > > > >> > >> > > > > > > > > >> > Don't fix if it works! >> > > > > > > > > >> > >> > > > > > > > > >> > If you 100% sure then it a useful addition to the >> > product >> > > - >> > > > > just >> > > > > > > > make >> > > > > > > > > a >> > > > > > > > > >> > separate ticket. >> > > > > > > > > >> > >> > > > > > > > > >> > В Ср, 25/04/2018 в 11:44 +0300, Vladimir Ozerov >> пишет: >> > > > > > > > > >> > > 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. >> > > > > > > > > >> > > > >> > > > > > > > > >> >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > >> > > > > >> > > > > -- >> > > > > Best regards, >> > > > > Andrey Kuznetsov. >> > > > > >> > > > >> > > >> > > >> > > >> > > -- >> > > Best regards, >> > > Andrey Kuznetsov. >> > > >> > >> > >
