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
