StefanMaron / BusinessCentral.LinterCop

Community driven code linter for AL (MS Dynamics 365 Business Central)
https://stefanmaron.com
MIT License
72 stars 30 forks source link

LC0047: Tok is meant to be used when the value of the label matches the name #545

Closed Arthurvdv closed 4 months ago

Arthurvdv commented 6 months ago

Based on https://github.com/microsoft/BCApps/pull/607#discussion_r1497208591, the current implementation of the LC0047 - Locked Label must have a suffix Tok. rule seems to be invalid.

Tok is meant to be used when the value of the label matches the name. As in: TokenTok: Label 'Token', Locked = true;

What is the best way forward? A) Adapt the current rule B) Create a new rule and lower the default Severity of the current LC0047 rule tot None

Some examples below, where the rule then should raise.

TokenTok: Label 'Token', Locked = true;
TokenLbl: Label 'Token', Locked = true; //LC00xx: Tok is meant to be used when the value of the label matches the name
TokenTxt: Label 'Token', Locked = true; //LC00xx: Tok is meant to be used when the value of the label matches the name

SimulationModeStartedTxt: Label 'No. Series simulation mode started.', Locked = true;  //LC00xx: Tok is meant to be used when the value of the label matches the name

AmpersandTok: Label '&', Locked = true; //LC00xx: Tok is meant to be used when the value of the label matches the name
NotEqualTok: Label '<>', Locked = true; //LC00xx: Tok is meant to be used when the value of the label matches the name
Arthurvdv commented 6 months ago
AmpersandTok: Label '&', Locked = true;

Is my assumption right that this should be treated as a token?
Renaming the label to &Tok isn't going to work for obvious reasons. I'm thinking of excluding the rule for labels with one (1) single char which is not letter (A-Za-z).

An other approach could be that a if a Label has the value '&' and Locked = true, then the naming of the Label should be AmpersandTok? I'm not sure if dictating how the label should be called is the path that we want to take 🤔

NKarolak commented 6 months ago

Difficult. I actually liked LC0047 the way it was, it made sense to me: whenever I see a label ending on Tok, I instantly know that it is a constant. But as long as Microsoft does not handle it that way, we should not do either. Furthermore, I'd implement Microsoft's way for LC0047 instead, but as info only. Whenever Tok is used, however, then the label must be locked (LC0046).

Is my assumption right that this should be treated as a token? Renaming the label to &Tok isn't going to work for obvious reasons. I'm thinking of excluding the rule for labels with one (1) single char which is not letter (A-Za-z).

👍

An other approach could be that a if a Label has the value '&' and Locked = true, then the naming of the Label should be AmpersandTok? I'm not sure if dictating how the label should be called is the path that we want to take 🤔

👎 (for any labels containing special signs)

StefanMaron commented 6 months ago

I agree with @NKarolak that I like the simplicity of if Locked then Tok.

I do not agree with this in general:

But as long as Microsoft does not handle it that way, we should not do either.

Not everything they do is always though through completely.

Now, I could not think about any example where a token would not be locked. If they are cases, those seem to be very rare and therefore qualify for a #pragma

More often, I would think, we could see cases where a non-Token Label would be locked. In this case, the original definition of the rule would raise a false positive, if we want to follow MS on this one.

And maybe we should just do two separate rules

  1. Label suffixed with Tok must be Locked = true
  2. Locked = true must be suffixed with Tok

Then I just need to decide which Rule I want to use. Personally, I would probably prefer the first option.

My goal with all the linter rules always was to keep the number of pramas needed as low as possible. If the rule does just give some guideline infos and is not build to warn against runtime errors or something, I would rather raise the rule on "only" 80% of the cases, rather then going for 110%

NKarolak commented 6 months ago

Not everything they do is always though through completely.

I agree, and that was not my message. My message was: If we define to see Tok as a constant, then we won't be able to interpret Microsoft's code correctly. We'd need to read it differently.

Label suffixed with Tok must be Locked = true

This rule already exists and is not in question: https://github.com/StefanMaron/BusinessCentral.LinterCop/wiki/LC0046

StefanMaron commented 6 months ago

Then I guess this is only about when should a Label be suffixed with Tok. And I think I would keep it as it was.

This would not be the only rule that fires a lot when ran against Microsoft code

Arthurvdv commented 6 months ago

Thank you both for joining the discussion here!

It think we all agree on that we can keep LC0046 as is, so we can focus on the implementation of LC0047 rule. I would suggest to keep the LC0047 rule as-is, where I've taken the liberty to lower the Default Severity from Info to None. In this way it can still be used. But as default the LinterCop isn't going to raise this diagnostic, unless specifically configured with the Custom.ruleset.json.

Additional I could create a new rule: LC00xx: The suffix Tok is meant to be used when the value of the label matches the name.

This rule wil check if the value of the Label matches with the name of the label without the suffix. So in the example below it wil suggest to change the name of TokenLbl or TokenTxt to TokenTok.

TokenTok: Label 'Token';
TokenLbl: Label 'Token'; //LC00xx: The suffix Tok is meant to be used when the value of the label matches the name
TokenTxt: Label 'Token'; //LC00xx: The suffix Tok is meant to be used when the value of the label matches the name