> No validation in the world prevent me from typing manually "mess" instead of > "msg"/«message"
Code review will do. > btw "svc" is more common imo Agree. I think you can start a discussion and change some abbrevs if you wish. > 17 июня 2021 г., в 10:23, Ivan Daschinsky <ivanda...@gmail.com> написал(а): > > I'm sorry, but the rule is not strict and, moreover, not clear enough for > constans. See here [1] > ``` > Type and method names are usually not abbreviated (except for the > well-accepted abbreviations such as EOF, Impl, Fifo, etc.). > > Abbreviation applies to local variables, method parameters, class fields > and in **some cases public static fileds** (constants) of the class. > ``` > But there is not any list that contains these cases. I suppose, that > constant naming should not be abbreviated. > As I see, the most cases of violations, described in initial post, are > about constant's namings. > > I suppose that we should define strict and clear rules about constants > naming. > > [1] -- https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules > > > чт, 17 июн. 2021 г. в 10:10, Konstantin Orlov <kor...@gridgain.com>: > >> Enforced validation doesn't guarantee code consistency. No validation in >> the world prevent me from typing manually "mess" instead of "msg"/"message" >> or "svc" instead of "srvc"/"service" (btw "svc" is more common imo). >> >> And the fact that someone name a variable "count" instead of "cnt" doesn't >> make the whole project immature. >> >> -- >> Regards, >> Konstantin Orlov >> >> >> >> >>> On 17 Jun 2021, at 08:34, Ivan Pavlukhin <vololo...@gmail.com> wrote: >>> >>> Hi Nikolay, >>> >>> Thanks, it is rather interesting. >>> >>> Do we all agree that using different conventions for different code >>> packages does not break "consistency"? Or did I get something wrong? >>> >>> 2021-06-17 7:12 GMT+03:00, Николай Ижиков <nizhi...@apache.org>: >>>> 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 >>>> >>> >>> >>> -- >>> >>> Best regards, >>> Ivan Pavlukhin >> >> > > -- > Sincerely yours, Ivan Daschinskiy
