Nikita, Do you have a plan in your mind how to make Ignite codebase consistent?
AFAIR, we had it intentionally inconsistent for a long time at least for one sake: for internal code we used special conventions (e.g. abbreviations, shorten getters) and common Java conventions for public API and examples (e.g. no abbreviations and usual getters). 2021-06-16 23:37 GMT+03:00, Nikita Ivanov <niva...@gridgain.com>: > Consistency is what makes it easier to contribute to the project and > attract new members. Consistency implies strong, well defined and > universally enforced rules. Just because we have some individuals who > are lazy or inexperienced - it does not mean that the entire project > should relax the basic-level engineering discipline. > > On a personal node - nothing screams "immaturity" louder that a code > that uses inconsistent naming, commenting, code style & organization, > etc. > -- > Nikita Ivanov > > On Wed, Jun 16, 2021 at 5:56 AM Andrey Gura <ag...@apache.org> wrote: >> >> Hi, >> >> I understand that this rule seems too strict or may be weird. But >> removing the rule could lead to review comments like "use future >> instead of fut". So my proposal is to change rule from "required" to >> "recommended". >> >> On Wed, Jun 16, 2021 at 2:49 PM Valentin Kulichenko >> <valentin.kuliche...@gmail.com> wrote: >> > >> > Konstantin, thanks for chiming in. >> > >> > That's exactly my concern. Overformalization is generally not a good >> > thing. >> > Someone used "mess" to abbreviate "message"? That is surely not a good >> > coding style, but that's what code reviews are for. I believe that our >> > committers are more than capable of identifying such issues. >> > >> > At the same time, with the current rules, we are *forced* to use >> > abbreviations like "locAddrGrpMgr", whether we like it or not. All it >> > does >> > is makes it harder to contribute to Ignite, without providing any clear >> > value. >> > >> > -Val >> > >> > On Thu, Jun 10, 2021 at 9:46 AM Konstantin Orlov <kor...@gridgain.com> >> > wrote: >> > >> > > +1 for making this optional >> > > >> > > Common abbreviation rules is good for sure, but sometimes I getting >> > > sick >> > > of all those locAddrGrpMgr. >> > > >> > > >> > > -- >> > > Regards, >> > > Konstantin Orlov >> > > >> > > >> > > >> > > >> > > > On 7 Jun 2021, at 14:31, Nikolay Izhikov <nizhi...@apache.org> >> > > > wrote: >> > > > >> > > > Hello, Anton, Alexei. >> > > > >> > > > Thanks for the feedback. >> > > > >> > > > Personally, I’m pretty happy current abbreviation rules too. >> > > > Let see what we can do to make our codebase even more consistent. >> > > > >> > > >> 7 июня 2021 г., в 13:23, Alexei Scherbakov < >> > > alexey.scherbak...@gmail.com> написал(а): >> > > >> >> > > >> -1 >> > > >> Common abbrevs add quality to the code. >> > > >> >> > > >> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <a...@apache.org>: >> > > >> >> > > >>> -1 here. >> > > >>> We can fix the code and set up the rule. >> > > >>> >> > > >>> This will help to prevent having a weird abbreviation like "mess" >> > > >>> (from >> > > >>> "message") or "ign" (from "Ignite"). >> > > >>> Also, the abbreviations list (hardcoded at IDEA plugin) allows to >> > > >>> have >> > > same >> > > >>> names across the whole code, this should simplify the reading. >> > > >>> >> > > >>> On Sat, Jun 5, 2021 at 10:49 PM Valentin Kulichenko < >> > > >>> valentin.kuliche...@gmail.com> wrote: >> > > >>> >> > > >>>> I also support removing this requirement. It’s not the first >> > > >>>> time >> > > someone >> > > >>>> brings this up, and so far we haven’t been able to fix it. Not >> > > >>>> worth >> > > it >> > > >>> in >> > > >>>> my view. >> > > >>>> >> > > >>>> -Val >> > > >>>> >> > > >>>> On Sat, Jun 5, 2021 at 11:54 AM Nikolay Izhikov >> > > >>>> <nizhi...@apache.org> >> > > >>>> wrote: >> > > >>>> >> > > >>>>> Hello, guys. >> > > >>>>> >> > > >>>>> Thanks for the feedback. >> > > >>>>> >> > > >>>>> Dmitry, >> > > >>>>> >> > > >>>>>> Manual rule enforcement saves a couple of symbols in code >> > > >>>>> >> > > >>>>> I’m talking about automatic check. >> > > >>>>> We can implement it easily. >> > > >>>>> So, if we decide to keep this rule all we need is to fix >> > > >>>>> current >> > > >>>>> violations (several thousand!). >> > > >>>>> After it, checkstyle will automatically enforce this rule. >> > > >>>>> As you may know, currently, any PR checked according to our >> > > checkstyle >> > > >>>>> rules. >> > > >>>>> Please, take a look at little green check sign after PR name. >> > > >>>>> >> > > >>>>> >> > > >>>>>> 5 июня 2021 г., в 00:57, Dmitry Pavlov <dpav...@apache.org> >> > > >>>> написал(а): >> > > >>>>>> >> > > >>>>>> +1 for removal. Cnt and count both seem to be fine. >> > > >>>>>> >> > > >>>>>> Manual rule enforcement saves a couple of symbols in code, but >> > > >>> requires >> > > >>>>> some time from both contributor and reviewer. >> > > >>>>>> >> > > >>>>>> Sincerely, >> > > >>>>>> Dmitriy Pavlov >> > > >>>>>> >> > > >>>>>> On 2021/06/04 19:18:37, Pavel Tupitsyn <ptupit...@apache.org> >> > > wrote: >> > > >>>>>>> In my opinion, we should remove this rule. >> > > >>>>>>> Looks like a waste of time. freq or frequency, cnt or count, >> > > >>>>>>> it is >> > > >>>> fine >> > > >>>>>>> either way. >> > > >>>>>>> >> > > >>>>>>> On Fri, Jun 4, 2021 at 7:39 PM Nikolay Izhikov < >> > > nizhi...@apache.org >> > > >>>> >> > > >>>>> wrote: >> > > >>>>>>> >> > > >>>>>>>> Hello, Igniters. >> > > >>>>>>>> >> > > >>>>>>>> Right now, we have the rule to use some predefined list of >> > > >>>> abbrevation >> > > >>>>> for >> > > >>>>>>>> variable names [1]. >> > > >>>>>>>> Some of the reviewers ask to follow this rule strictly. >> > > >>>>>>>> >> > > >>>>>>>>> It is required to use abbreviated form for code >> > > >>>>>>>>> consistency. >> > > >>>>>>>> >> > > >>>>>>>> I tried to implement this rule in form of checkstyle check >> > > >>>>>>>> [2] and >> > > >>> it >> > > >>>>>>>> seems like many of use doesn’t follow this requirement. >> > > >>>>>>>> My check found 4124 violation in core module. >> > > >>>>>>>> >> > > >>>>>>>> Should we make this rule optional in documentation or should >> > > >>>>>>>> we >> > > >>>> remove >> > > >>>>> it >> > > >>>>>>>> completely? >> > > >>>>>>>> Or should someone proceed and fix all the violations? >> > > >>>>>>>> >> > > >>>>>>>> WDYT? >> > > >>>>>>>> >> > > >>>>>>>> >> > > >>>>>>>> Example of output: >> > > >>>>>>>> ``` >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/IgnitePdsWithTtlTest.java:94: >> > > >>>>>>>> Abbrevation should be used for CACHE_NAME_LOCAL_ATOMIC! >> > > >>>>>>>> Please, >> > > use >> > > >>>>> loc, >> > > >>>>>>>> instead of LOCAL [IgniteAbbrevationsRule] >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/IgnitePdsWithTtlTest.java:97: >> > > >>>>>>>> Abbrevation should be used for CACHE_NAME_LOCAL_TX! Please, >> > > >>>>>>>> use >> > > >>> loc, >> > > >>>>>>>> instead of LOCAL [IgniteAbbrevationsRule] >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/IgnitePdsWithTtlTest.java:264: >> > > >>>>>>>> Abbrevation should be used for checkpointManager! Please, use >> > > >>>>>>>> mgr, >> > > >>>>> instead >> > > >>>>>>>> of Manager [IgniteAbbrevationsRule] >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/IgnitePdsPartitionPreloadTest.java:63: >> > > >>>>>>>> Abbrevation should be used for DEFAULT_REGION! Please, use >> > > >>>>>>>> dflt, >> > > >>>>> instead of >> > > >>>>>>>> DEFAULT [IgniteAbbrevationsRule] >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/IgnitePdsWholeClusterRestartTest.java:47: >> > > >>>>>>>> Abbrevation should be used for ENTRIES_COUNT! Please, use >> > > >>>>>>>> cnt, >> > > >>>> instead >> > > >>>>> of >> > > >>>>>>>> COUNT [IgniteAbbrevationsRule] >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/IgnitePdsRebalancingOnNotStableTopologyTest.java:49: >> > > >>>>>>>> Abbrevation should be used for CHECKPOINT_FREQUENCY! Please, >> > > >>>>>>>> use >> > > >>>> freq, >> > > >>>>>>>> instead of FREQUENCY [IgniteAbbrevationsRule] >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/IgnitePdsTransactionsHangTest.java:75: >> > > >>>>>>>> Abbrevation should be used for MAX_KEY_COUNT! Please, use >> > > >>>>>>>> cnt, >> > > >>>> instead >> > > >>>>> of >> > > >>>>>>>> COUNT [IgniteAbbrevationsRule] >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/IgnitePdsTransactionsHangTest.java:78: >> > > >>>>>>>> Abbrevation should be used for CHECKPOINT_FREQUENCY! Please, >> > > >>>>>>>> use >> > > >>>> freq, >> > > >>>>>>>> instead of FREQUENCY [IgniteAbbrevationsRule] >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/SlowHistoricalRebalanceSmallHistoryTest.java:57: >> > > >>>>>>>> Abbrevation should be used for SUPPLY_MESSAGE_LATCH! Please, >> > > >>>>>>>> use >> > > >>> msg, >> > > >>>>>>>> instead of MESSAGE [IgniteAbbrevationsRule] >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/IgniteLogicalRecoveryTest.java:74: >> > > >>>>>>>> Abbrevation should be used for SHARED_GROUP_NAME! Please, use >> > > >>>>>>>> grp, >> > > >>>>> instead >> > > >>>>>>>> of GROUP [IgniteAbbrevationsRule] >> > > >>>>>>>> [ERROR] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > /Users/sbt-izhikov-nv/work/ignite/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/IgniteLogicalRecoveryTest.java:200: >> > > >>>>>>>> Abbrevation should be used for cacheLoader! Please, use ldr, >> > > >>> instead >> > > >>>> of >> > > >>>>>>>> Loader [IgniteAbbrevationsRule] >> > > >>>>>>>> ``` >> > > >>>>>>>> >> > > >>>>>>>> [1] >> > > >>>>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules#AbbreviationRules-VariableAbbreviation >> > > >>>>>>>> [2] https://github.com/apache/ignite/pull/9153 >> > > >>>>>>>> >> > > >>>>>>>> >> > > >>>>>>> >> > > >>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > >> >> > > >> >> > > >> -- >> > > >> >> > > >> Best regards, >> > > >> Alexei Scherbakov >> > > > >> > > >> > > > -- Best regards, Ivan Pavlukhin
