Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The name of TableName is not valid. The name of variables and parameters must be suffixed with the type or object name.AL(AA0072) #6120

Closed
BartPermentier opened this issue Sep 2, 2020 · 23 comments
Labels
bug Product bug CodeCop This is a specific static-code-analysis group (AA) in-progress static-code-analysis

Comments

@BartPermentier
Copy link

Describe the bug
Could this warning Ignore the Prefix of the Object please? It's just incredibly ugly to add the prefix to ObjectNames.

To Reproduce
Steps and to reproduce the behavior:
Add variables with a Prefix:

AL Code to reproduce the issue

var
        WebRequestLog: Record "ESC Web Request Log";
        StartProcessResponse: Codeunit "ESC Start Process Response";

Expected behavior
Don't trigger the warning if only the prefix is missing

Screenshots
If applicable, add screenshots to help explain your problem.
image

5. Versions:

  • AL Language: 6.0.324421
  • Business Central: Not related to a BC Version
@dzzzb
Copy link

dzzzb commented Sep 2, 2020

Same would go for suffixes if they are also only used to mark fields as belonging to a co./app.

@thpeder thpeder added CodeCop This is a specific static-code-analysis group (AA) static-code-analysis labels Sep 2, 2020
@jimmymcp
Copy link

jimmymcp commented Sep 2, 2020

Agree with dtkb - we use suffixes for AppSource. Suddenly we've got hundreds of suggestions that our variable names aren't valid 😱

@IvoMol
Copy link

IvoMol commented Sep 2, 2020

Two more problems with this CodeCop:

1) Problem in [EventSubscriber] procedure
[EventSubscriber(ObjectType::Table, Database::Contact, 'OnAfterDeleteEvent', '', false, false)]
local procedure DeleteLogOnAfterDelete(var Rec: Record Contact)
Cannot be changed because TableName must follow publisher.

2) Problem if object name contains dot
var
LineFeeNoteOnReportHist: Record "Line Fee Note on Report Hist.";
GetDocumentNoAndDate: Page "Get Document No. and Date";

@AskeHolst
Copy link

Even common abbreviations are a problem
WhseJnlTemplate: Record "Warehouse Journal Template";
image

And when combined with the rule for temporary (Temp prefix) it gets very verbose. Any unique part of the name must be inside:
NewWarehouseJournalTemplateTemp: Record "Warehouse Journal Template" temporary; // Not allowed TempWarehouseJournalTemplateOld: Record "Warehouse Journal Template" temporary; // Not allowed TempNewWarehouseJournalTemplate, TempOldWarehouseJournalTemplate: Record "Warehouse Journal Template" temporary; // As required
I'm all for standards and verbose names, but this gets less readable. Yes it can be suppressed, but the general idea is good and I want the rule, it's just a little too hard.

@IvoMol
Copy link

IvoMol commented Sep 2, 2020

How to correct name two variables the same record? Above that temporary?
var
Customer1: Record Customer;
Customer2: Record Customer;

@dzzzb
Copy link

dzzzb commented Sep 2, 2020

It sounds like this feature was just nowhere near ready enough to be released. I bet running it over the standard codebase, which for better or worse is a fairly good predictor of a lot of end-user code out there, would have noisy enough results to make that clear.

Even after the bugs were fixed and convenience features added, how could we reconcile this with the typical idiom of abbreviating Journal to Jnl, Purchase to Purch, etc.? Either someone has to maintain a list of 'approved' abbreviations, or you basically say that we can't abbreviate anymore, and in the latter case readability arguably goes way down - and certainly a lot of version control diff noise results from 'correcting' the code (or we have yet another practically unusable warning to suppress).

@IvoMol
Copy link

IvoMol commented Sep 3, 2020

I asked the same question about abbreviations a few months ago, but without an answer.
Therefore, we had to rename all the abbreviated variables in the whole application (hundreds of occurrences). It was hours of work and the readability of the code did not improve.

A similar question was if I had a Record variable (such as Customer) once as local, the second as global: it must not be named the same, but both must have the suffix TableName. Will we introduce the (previously rejected) prefixes Loc... and Glob...?

@MarcHansenMicrosoft
Copy link
Contributor

Not having Prefix and Suffix in the name should be allowed.
That is a bug.

EventSubcriber error is a bug as well.
. and other special chars is a bug as well.

We are working on the Temp issue.

Not allowing abbreviations are by design. We had a big internal discussion and landed on no abbreviations.

Not allowing 2 vars like customer1 and customer2 is also a bug.

Thanks for all the good feedbask.

@pieter-kok
Copy link

I'm curious about whether Microsoft will also update the base app to apply to this rule. The reason I'm addressing this is because reports need to be copied to our own extensions to be customized. To apply to this rule we would now have to update the variable names in the copied report. When the report is updated later on in the base app, we would in many cases also like to update our copied report(s) which is an easier task when all variable names match.

It's good to hear that the Temp issue is worked on, I agree to the comment of @AskeHolst about the readability. Is it also possible to allow both 'Buffer' and 'Temp' for temporary records?

@jimmymcp
Copy link

jimmymcp commented Sep 3, 2020

@MarcHansenMicrosoft thanks for the feedback. I'm curious though why MS feel the need to dictate any rules at all on this?

I've got a few concerns:

  • there will always be (what I would consider) false positives for sensible names e.g. WhseJnlLine is perfectly understandable and used in the base code but will be considered invalid
  • conversely, I sometimes have to abbreviate the object name due to the restriction on length and make my variable names more descriptive than the object name
  • at the moment our code analysis is so noisy with invalid variable warnings that we might miss something more important

I know I can just disable this rule but I'm interested why we've got it at all.

@NKarolak
Copy link

NKarolak commented Sep 3, 2020

I totally agree with @jimmymcp .

Just one more thought (sorry if already mentioned somewhere):
Since we (soon) need to replace all explicit "with"s in code with the full record name, we will be sometimes forced to use short or even extra short record variable names to keep the code readable.

@MarcHansenMicrosoft
Copy link
Contributor

It will be only Temp for now.

We will not force this rule for BaseApp only System and other extension.

For copied reports I would pragma ignore the specific rule.

I see all of your points in the abbreviations but for now we do not plan to change it.

First we will focus on the bugs.

@MattTraxinger
Copy link

I see there have been some new threads started, but I'm surprised there hasn't been a comment here in a month. I'll be the first to say that I am all for standards and nudging, sometimes forcing, developers in the direction of improving code quality. I absolutely love the majority of the rules. Unfortunately I feel that this one is forcing a reduction in quality.

I have to imagine the reason that there isn't a "Suggested Fix" for the warning is that Microsoft does not have the ability to define what "Readable" code actually is. The only fix that would work every time would make the code so unreadable it would destroy the intended results. So we're left with the developer making yet another choice on readability. Does anyone else see the irony here?

Variable naming is a standard, not a rule. We'll continue to educate developers on the standards for properly naming their variables, and they will adapt specific situations to write working code that is still readable. No one can put a firm rule on what readable code is. It's subjective. I haven't seen a rash of unreadable code lately, so unfortunately this is probably going to fall into the ever increasing set of ignores.

Could someone from Microsoft explain what root problem was occurring and why this was the solution? My mind could definitely be changed.

@yurim108
Copy link

yurim108 commented Oct 5, 2020

Some more issues:

  • RecRef: RecordRef; // using RecordRef as a name is weird, since it is highlighted as a reserved word
  • same with FieldRef and KeyRef

@IvoMol
Copy link

IvoMol commented Oct 5, 2020

same with InStr and OutStr

@ghost
Copy link

ghost commented Oct 5, 2020

I asked the same question about abbreviations a few months ago, but without an answer.
Therefore, we had to rename all the abbreviated variables in the whole application (hundreds of occurrences). It was hours of work and the readability of the code did not improve.

A similar question was if I had a Record variable (such as Customer) once as local, the second as global: it must not be named the same, but both must have the suffix TableName. Will we introduce the (previously rejected) prefixes Loc... and Glob...?

I am using a suffix 'G' for global variables. For 2 reasons: 1. make a distinction between a local var like Customer, and a global var Customer, and 2. to make very clear to any programmer that a suffix 'G' stands for global var (I only use the Hungarian notation for this case).
But since an update to v7.0.341259 I get the AA0072 info problem. Annoying.

@IvoMol
Copy link

IvoMol commented Oct 5, 2020

"Not having Prefix and Suffix in the name should be allowed."
WHAT? We renamed hundreds of occurrences in accordance with this rule. Was it useless?

Please finalize this role ASAP. Abbreviations allowed yes/no. Prefixes/suffixes mandatory yes/no. Object name as suffix yes/no. Reserved words yes/no. Numbers in variable names yes/no. Disable in subscriber parameters.

Thanks.

@MattTraxinger
Copy link

To revisit my comments from last week, I am glad that this rule is in there as an option. That said, I think we've pretty much gotten to where we are going to add this to our ignore file and the more I think about it the more I think we might not ever take it out.

It's been this way for a while, and I do at least partly understand why, but the idea that these rules are not enforced in the main codebase just doesn't feel right to me. I get that every company has to make its own decisions about what quality means to them and what to prioritize. At the same time, combined with the suggestion to pragma ignore the warnings when copying from a base report instead of fixing them? And the insufficient testing before this rule was released? To me that just shows that this isn't a serious issue for Microsoft, and thus not a serious issue for me.

@aptMattKoe
Copy link

aptMattKoe commented Oct 8, 2020

I really do not understand all restrictions.
But I use this Issue because its the same Error but not with Tables.

image

Whats with those Datatypes like Dialog or Variant? Its a bug or?

@paul-lbmc
Copy link

Is there a reason this rule is case sensitive? Nothing else in AL is...

@dan-cris
Copy link

Hi everyone, some changes were made to AA0072:
@BartPermentier
Affixes (such as ESC in your example, but applies to suffixes as well) no longer have to be included in the variable name. The examples you gave should work now

@IvoMol
The issue with EventSubscribers has been fixed.
The dot character does not trigger the rule, it's the fact that the rule was case sensitive. That has since been changed.
The rule was also relaxed to allow suffixing variables in order to differentiate between variables of the same record (such as CustomerNew & CustomerOld, or Customer1 & Customer2)

@NKarolak
Copy link

NKarolak commented Oct 15, 2020

Thanks @dan-cris , from which AL Language version on is it available?

@PaulIvan
Copy link

PaulIvan commented Oct 15, 2020

@NKarolak it should be available in the latest developer preview .

I will close this issue as it is diverging from the original. If you still have issues after trying the developer preview please make another issue and report it there 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Product bug CodeCop This is a specific static-code-analysis group (AA) in-progress static-code-analysis
Projects
None yet
Development

No branches or pull requests