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
