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