[DISCUSSION] Code style. Variable abbrevations

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

[DISCUSSION] Code style. Variable abbrevations

Nikolay Izhikov-2
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

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Pavel Tupitsyn
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 <[hidden email]> 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
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Dmitry Pavlov
+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 <[hidden email]> 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 <[hidden email]> 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
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Nikolay Izhikov-2
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 <[hidden email]> написал(а):
>
> +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 <[hidden email]> 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 <[hidden email]> 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
>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Valentin Kulichenko
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 <[hidden email]> 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 <[hidden email]> написал(а):
> >
> > +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 <[hidden email]> 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 <[hidden email]>
> 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
> >>>
> >>>
> >>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Anton Vinogradov-2
-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 <
[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]>
> написал(а):
> > >
> > > +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 <[hidden email]> 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 <[hidden email]>
> > 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
> > >>>
> > >>>
> > >>
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Alexei Scherbakov
-1
Common abbrevs add quality to the code.

пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:

> -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 <
> [hidden email]> 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 <[hidden email]>
> > 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 <[hidden email]>
> > написал(а):
> > > >
> > > > +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 <[hidden email]> 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 <[hidden email]
> >
> > > 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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Nikolay Izhikov-2
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 <[hidden email]> написал(а):
>
> -1
> Common abbrevs add quality to the code.
>
> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
>
>> -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 <
>> [hidden email]> 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 <[hidden email]>
>>> 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 <[hidden email]>
>>> написал(а):
>>>>>
>>>>> +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 <[hidden email]> 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 <[hidden email]
>>>
>>>> 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

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Konstantin Orlov
+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 <[hidden email]> 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 <[hidden email]> написал(а):
>>
>> -1
>> Common abbrevs add quality to the code.
>>
>> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
>>
>>> -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 <
>>> [hidden email]> 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 <[hidden email]>
>>>> 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 <[hidden email]>
>>>> написал(а):
>>>>>>
>>>>>> +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 <[hidden email]> 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 <[hidden email]
>>>>
>>>>> 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
>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Valentin Kulichenko
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 <[hidden email]>
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 <[hidden email]> 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 <
> [hidden email]> написал(а):
> >>
> >> -1
> >> Common abbrevs add quality to the code.
> >>
> >> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
> >>
> >>> -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 <
> >>> [hidden email]> 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 <[hidden email]>
> >>>> 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 <[hidden email]>
> >>>> написал(а):
> >>>>>>
> >>>>>> +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 <[hidden email]>
> 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 <
> [hidden email]
> >>>>
> >>>>> 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
> >
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

agura
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
<[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]> 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 <
> > [hidden email]> написал(а):
> > >>
> > >> -1
> > >> Common abbrevs add quality to the code.
> > >>
> > >> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
> > >>
> > >>> -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 <
> > >>> [hidden email]> 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 <[hidden email]>
> > >>>> 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 <[hidden email]>
> > >>>> написал(а):
> > >>>>>>
> > >>>>>> +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 <[hidden email]>
> > 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 <
> > [hidden email]
> > >>>>
> > >>>>> 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
> > >
> >
> >
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Nikita Ivanov-2
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 <[hidden email]> 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
> <[hidden email]> 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 <[hidden email]>
> > 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 <[hidden email]> 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 <
> > > [hidden email]> написал(а):
> > > >>
> > > >> -1
> > > >> Common abbrevs add quality to the code.
> > > >>
> > > >> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
> > > >>
> > > >>> -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 <
> > > >>> [hidden email]> 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 <[hidden email]>
> > > >>>> 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 <[hidden email]>
> > > >>>> написал(а):
> > > >>>>>>
> > > >>>>>> +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 <[hidden email]>
> > > 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 <
> > > [hidden email]
> > > >>>>
> > > >>>>> 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
> > > >
> > >
> > >
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Ivan Pavlukhin
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 <[hidden email]>:

> 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 <[hidden email]> 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
>> <[hidden email]> 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 <[hidden email]>
>> > 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 <[hidden email]>
>> > > > 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 <
>> > > [hidden email]> написал(а):
>> > > >>
>> > > >> -1
>> > > >> Common abbrevs add quality to the code.
>> > > >>
>> > > >> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
>> > > >>
>> > > >>> -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 <
>> > > >>> [hidden email]> 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
>> > > >>>> <[hidden email]>
>> > > >>>> 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 <[hidden email]>
>> > > >>>> написал(а):
>> > > >>>>>>
>> > > >>>>>> +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 <[hidden email]>
>> > > 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 <
>> > > [hidden email]
>> > > >>>>
>> > > >>>>> 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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Nikolay Izhikov-2
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 <[hidden email]> написал(а):
>
> 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 <[hidden email]>:
>> 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 <[hidden email]> 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
>>> <[hidden email]> 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 <[hidden email]>
>>>> 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 <[hidden email]>
>>>>>> 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 <
>>>>> [hidden email]> написал(а):
>>>>>>>
>>>>>>> -1
>>>>>>> Common abbrevs add quality to the code.
>>>>>>>
>>>>>>> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
>>>>>>>
>>>>>>>> -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 <
>>>>>>>> [hidden email]> 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
>>>>>>>>> <[hidden email]>
>>>>>>>>> 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 <[hidden email]>
>>>>>>>>> написал(а):
>>>>>>>>>>>
>>>>>>>>>>> +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 <[hidden email]>
>>>>> 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 <
>>>>> [hidden email]
>>>>>>>>>
>>>>>>>>>> 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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Ivan Pavlukhin
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, Николай Ижиков <[hidden email]>:

> 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 <[hidden email]>
>> написал(а):
>>
>> 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 <[hidden email]>:
>>> 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 <[hidden email]> 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
>>>> <[hidden email]> 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 <[hidden email]>
>>>>> 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 <[hidden email]>
>>>>>>> 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 <
>>>>>> [hidden email]> написал(а):
>>>>>>>>
>>>>>>>> -1
>>>>>>>> Common abbrevs add quality to the code.
>>>>>>>>
>>>>>>>> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
>>>>>>>>
>>>>>>>>> -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 <
>>>>>>>>> [hidden email]> 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
>>>>>>>>>> <[hidden email]>
>>>>>>>>>> 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 <[hidden email]>
>>>>>>>>>> написал(а):
>>>>>>>>>>>>
>>>>>>>>>>>> +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 <[hidden email]>
>>>>>> 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 <
>>>>>> [hidden email]
>>>>>>>>>>
>>>>>>>>>>> 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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Konstantin Orlov
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 <[hidden email]> 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, Николай Ижиков <[hidden email]>:
>> 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 <[hidden email]>
>>> написал(а):
>>>
>>> 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 <[hidden email]>:
>>>> 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 <[hidden email]> 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
>>>>> <[hidden email]> 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 <[hidden email]>
>>>>>> 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 <[hidden email]>
>>>>>>>> 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 <
>>>>>>> [hidden email]> написал(а):
>>>>>>>>>
>>>>>>>>> -1
>>>>>>>>> Common abbrevs add quality to the code.
>>>>>>>>>
>>>>>>>>> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
>>>>>>>>>
>>>>>>>>>> -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 <
>>>>>>>>>> [hidden email]> 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
>>>>>>>>>>> <[hidden email]>
>>>>>>>>>>> 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 <[hidden email]>
>>>>>>>>>>> написал(а):
>>>>>>>>>>>>>
>>>>>>>>>>>>> +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 <[hidden email]>
>>>>>>> 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 <
>>>>>>> [hidden email]
>>>>>>>>>>>
>>>>>>>>>>>> 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

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Ivan Daschinsky
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 <[hidden email]>:

> 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 <[hidden email]> 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, Николай Ижиков <[hidden email]>:
> >> 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 <[hidden email]>
> >>> написал(а):
> >>>
> >>> 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 <[hidden email]>:
> >>>> 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 <[hidden email]>
> 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
> >>>>> <[hidden email]> 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 <
> [hidden email]>
> >>>>>> 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 <[hidden email]>
> >>>>>>>> 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 <
> >>>>>>> [hidden email]> написал(а):
> >>>>>>>>>
> >>>>>>>>> -1
> >>>>>>>>> Common abbrevs add quality to the code.
> >>>>>>>>>
> >>>>>>>>> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
> >>>>>>>>>
> >>>>>>>>>> -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 <
> >>>>>>>>>> [hidden email]> 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
> >>>>>>>>>>> <[hidden email]>
> >>>>>>>>>>> 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 <[hidden email]>
> >>>>>>>>>>> написал(а):
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +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 <[hidden email]
> >
> >>>>>>> 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 <
> >>>>>>> [hidden email]
> >>>>>>>>>>>
> >>>>>>>>>>>> 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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Nikolay Izhikov-2
> 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 <[hidden email]> написал(а):
>
> 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 <[hidden email]>:
>
>> 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 <[hidden email]> 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, Николай Ижиков <[hidden email]>:
>>>> 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 <[hidden email]>
>>>>> написал(а):
>>>>>
>>>>> 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 <[hidden email]>:
>>>>>> 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 <[hidden email]>
>> 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
>>>>>>> <[hidden email]> 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 <
>> [hidden email]>
>>>>>>>> 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 <[hidden email]>
>>>>>>>>>> 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 <
>>>>>>>>> [hidden email]> написал(а):
>>>>>>>>>>>
>>>>>>>>>>> -1
>>>>>>>>>>> Common abbrevs add quality to the code.
>>>>>>>>>>>
>>>>>>>>>>> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
>>>>>>>>>>>
>>>>>>>>>>>> -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 <
>>>>>>>>>>>> [hidden email]> 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
>>>>>>>>>>>>> <[hidden email]>
>>>>>>>>>>>>> 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 <[hidden email]>
>>>>>>>>>>>>> написал(а):
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +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 <[hidden email]
>>>
>>>>>>>>> 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 <
>>>>>>>>> [hidden email]
>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Anton Vinogradov-2
> Agree. I think you can start a discussion and change some abbrevs if you
wish.

How about keeping the abbreviation map file inside the Ignite repo?
In this case, you may change the abbreviation by issue/pr covered by a
green TC check.

On Thu, Jun 17, 2021 at 11:36 AM Nikolay Izhikov <[hidden email]>
wrote:

> > 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 <[hidden email]>
> написал(а):
> >
> > 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 <[hidden email]>:
> >
> >> 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 <[hidden email]> 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, Николай Ижиков <[hidden email]>:
> >>>> 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 <[hidden email]>
> >>>>> написал(а):
> >>>>>
> >>>>> 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 <[hidden email]>:
> >>>>>> 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 <[hidden email]>
> >> 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
> >>>>>>> <[hidden email]> 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 <
> >> [hidden email]>
> >>>>>>>> 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 <[hidden email]>
> >>>>>>>>>> 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 <
> >>>>>>>>> [hidden email]> написал(а):
> >>>>>>>>>>>
> >>>>>>>>>>> -1
> >>>>>>>>>>> Common abbrevs add quality to the code.
> >>>>>>>>>>>
> >>>>>>>>>>> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]>:
> >>>>>>>>>>>
> >>>>>>>>>>>> -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 <
> >>>>>>>>>>>> [hidden email]> 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
> >>>>>>>>>>>>> <[hidden email]>
> >>>>>>>>>>>>> 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 <[hidden email]
> >
> >>>>>>>>>>>>> написал(а):
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> +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 <
> [hidden email]
> >>>
> >>>>>>>>> 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 <
> >>>>>>>>> [hidden email]
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> 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
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSSION] Code style. Variable abbrevations

Pavel Kovalenko
I think in this discussion there should be a question - why do we still
need abbreviations for all variables and fields? What problem does it solve?
Nobody still clearly answered this question.
I saw only 2 arguments to keep abbreviations:
1) Less codebase symbols. I think it is a weak argument. We're not in the
80-90s when there were no modern IDEs with hints and auto-completion.
Ideally code should be read just as english text. That really helps to
understand the code if you see it for the first time. Abbreviations
everywhere (except well-known CS terms) don't improve code readability.
2) We shouldn't break constitency with old codebase written with enforced
abbreviations. Hence the question - why was this rule introduced at project
startup? Who knows that?
I guess most people who decided to introduce this rule left the Ignite
project.

>> Consistency is what makes it easier to contribute to the project
Yes it is. But what makes it easier is actually clear architecture, simple
and convenient abstractions and well-documented code. Ignite has a big lack
of that.
For example, look at the transactions or PME codebase. It's darkness and no
abbreviations for "consistency" will help to understand that code.

>> and attract new members
Who can tell me at least 5 contributors with 10+ commits who are not
working/worked in GridGain or SberBank?
It's sad to hide behind an open source community that really does not exist.


пт, 18 июн. 2021 г. в 14:21, Anton Vinogradov <[hidden email]>:

> > Agree. I think you can start a discussion and change some abbrevs if you
> wish.
>
> How about keeping the abbreviation map file inside the Ignite repo?
> In this case, you may change the abbreviation by issue/pr covered by a
> green TC check.
>
> On Thu, Jun 17, 2021 at 11:36 AM Nikolay Izhikov <[hidden email]>
> wrote:
>
> > > 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 <[hidden email]>
> > написал(а):
> > >
> > > 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 <[hidden email]>:
> > >
> > >> 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 <[hidden email]>
> 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, Николай Ижиков <[hidden email]>:
> > >>>> 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 <[hidden email]>
> > >>>>> написал(а):
> > >>>>>
> > >>>>> 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 <[hidden email]>:
> > >>>>>> 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 <[hidden email]>
> > >> 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
> > >>>>>>> <[hidden email]> 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 <
> > >> [hidden email]>
> > >>>>>>>> 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 <[hidden email]
> >
> > >>>>>>>>>> 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 <
> > >>>>>>>>> [hidden email]> написал(а):
> > >>>>>>>>>>>
> > >>>>>>>>>>> -1
> > >>>>>>>>>>> Common abbrevs add quality to the code.
> > >>>>>>>>>>>
> > >>>>>>>>>>> пн, 7 июн. 2021 г. в 12:38, Anton Vinogradov <[hidden email]
> >:
> > >>>>>>>>>>>
> > >>>>>>>>>>>> -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 <
> > >>>>>>>>>>>> [hidden email]> 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
> > >>>>>>>>>>>>> <[hidden email]>
> > >>>>>>>>>>>>> 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 <
> [hidden email]
> > >
> > >>>>>>>>>>>>> написал(а):
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> +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 <
> > [hidden email]
> > >>>
> > >>>>>>>>> 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 <
> > >>>>>>>>> [hidden email]
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>> 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
> >
> >
>