microsoft / AL

Home of the Dynamics 365 Business Central AL Language extension for Visual Studio Code. Used to track issues regarding the latest version of the AL compiler and developer tools available in the Visual Studio Code Marketplace or as part of the AL Developer Preview builds for Dynamics 365 Business Central.
MIT License
733 stars 243 forks source link

Remove / loosen the new rule AL0659 for enums #6740

Open fvet opened 3 years ago

fvet commented 3 years ago

Please include the following with each issue:

1. Describe the bug

We've been refactoring a LOT of our 'option' fields / variables the past year into new 'enum' objects. However, when compiling against the new insider builds, a LOT of our new enums result in AL0659 warnings, forcing us to REMOVE these enums again and creating new enums (again) with shorter names.... and obsoleting lots of our new procedures :(

Can you please reconsider to make the rule less strict and allow enums with longer (30+) names, just as like it's supported today?

2. To Reproduce Steps to reproduce the behavior:

  1. Create enum with long name (30+ chars)

enum 2002700 "NVT Invoicing Currency Based On"
{
    Extensible = false;

    value(0; Default)
    {
        Caption = 'Default';
    }
    value(1; "Service Currency")
    {
        Caption = 'Service Currency';
    }
    value(2; "Customer Currency")
    {
        Caption = 'Customer Currency';
    }
}

Compile with C:\bcartifacts.cache\sandbox\ 19.0.29285.0 \VSIX\extension\bin\alc.exe

Warning

`

[warning]Navitrans 365 Base Application: \Navitrans.Base\app\src\Base\ENUM Invoicing Currency Based On.al(1,14): warning AL0659: Length of the application object identifier 'NVT Invoicing Currency Based On' cannot exceed 30 characters This warning will become an error in a future release.

`

3. Expected behavior Remove / loosen the new AL0659 rule

4. Actual behavior

5. Versions:

nicolassaleron commented 3 years ago

I get the limitation on table and fields due to SQL Server limitation on object names...but why extending this limitation on other objects ?

The effort should go to the direction of letting dev naming object clearly.

nndobrev commented 3 years ago

Hello and thank you for your time logging the bug. We relaxed the warning error to just a warning temporarily due to the high usage of the construct and also because many partners are considering the warning errors as errors. This change will be available in the next minor 18 versions, however, from version 19 this warning error will be present and like any other warning error we plan to move it to an error in future releases. Partners are more than welcome to suppress it using suppressWarnings key in app.json until they are able to accommodate the change.

There is also a post on aka.ms/bcideas regarding this that is currently under review and you are more than free to upvote. https://experience.dynamics.com/ideas/idea/?ideaid=7dce671b-7802-ea11-b862-0003ff68edfa

pkoutsogilas commented 3 years ago

So now partners must invest maybe a lot of time to reduce the length of enums in their apps, but it could be that in the future it is allowed again? That does not make sense. Or I'm a missing something here? Why is this done?

thloke commented 3 years ago

Sorry for the confusion. We do not have any concrete plans to move this warning to an error at the moment. I'll try to clear up where we are at and the reasoning behind it.

The warning was initially introduced in response to #6691, which alerted us to the fact that we forgot to enforce the 30 character name limit on enums. This causes the object name to be truncated when it is stored in the database, and may cause runtime confusion if another enum with the same first 30 characters exists and the runtime decides to operate on their names.

I say may cause runtime confusion because we do not know if it actually will. As far as we know, it doesn't right now cause any runtime confusion, but we haven't exhaustively checked to be 100% sure that it will not, and even if it is working now, we cannot guarantee that it will continue to work in the future. To not break existing code, we made it initially a warning error, and after realizing that it might be sending the wrong message, now a warning.

There is also the aspect of being more consistent in the AL language - it's weird to have this one object type not be subject to the naming length restriction, but since it's more or less working and it'd be a breaking change, there was no need to hard enforce the limit. Consistency is nice, but not breaking partners without a good reason is even nicer.

Ideally we'd fix this by removing the 30 character limit on application object names altogether, but that is a larger piece of work. (You can help us prioritize it by voting on the idea on BCIdeas!) The warning was introduced to alert developers to the possibility that it might cause runtime issues because of the name truncation.

Moving forward, we're still internally discussing where we will go with this. If the work to remove the 30 character limit on all application object names gets prioritized in the near future, then all is good. If not, then we will have to do more investigation around the potential (if any) runtime issues of having two or more enums with the same first 30 characters is.

For now, if none of your enums share the first 30 characters, it's pretty safe to just ignore this warning. If you do have enums that share the first 30 characters, then it's worth trying to address the warning since it might cause a potential runtime issue. I do not think it's worth investing the time to reduce the length of your enums to 30 characters or less unless you're in the case where the first 30 characters are the same.

I can't say how likely it is we will convert this to an error in the future, we're still discussing this internally and have not reached a conclusion yet. It probably depends on if we can find any actual serious issue around having two objects in the DB with the same (truncated) name. If we do decide that it is necessary to make it an error, we will communicate this by updating the warning message to say when it will be made an error (I know it currently says that it will be made into an error, but we're walking that back to just be a regular warning with no commitment to make it an error).

tl;dr: don't worry about this unless you have enums that share the first 30 characters in their name.

pkoutsogilas commented 3 years ago

@thloke : Thank you for the clarification ! :)

waldo1001 commented 3 years ago

@thloke , It seems there are enough votes already ;-)

https://experience.dynamics.com/ideas/idea/?ideaid=7dce671b-7802-ea11-b862-0003ff68edfa

image

thloke commented 3 years ago

I agree too. I cannot give any specific promises on when it will be done, but I can say that it's been brought up internally for consideration. When will depend on how this stacks up against the other development priorities and the final estimated cost.

fartwhif commented 1 year ago

The priority of alleviating this ridiculous restriction should be elevated, and the problem should be taken more seriously. It feels like AL is designed by people who have a suffocation fetish.