Hello, Ivan. We can create checkstyle rule to enforce usage of abbreviations. Internal/public code differs by package.
PoC of rule [1] [1] https://github.com/apache/ignite/pull/9153 > 17 июня 2021 г., в 07:01, Ivan Pavlukhin <vololo...@gmail.com> написал(а): > > 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
