StefanMaron / BusinessCentral.LinterCop

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

LC0005 Avoid duplicate diagnostic, add diagnostic field syntax length #640

Closed ans-bar-bm closed 1 month ago

ans-bar-bm commented 1 month ago

Fixes #248 Fixes #639

Arthurvdv commented 1 month ago

Thank you for contributing and creating an pull-request for the both issues.

I've recently started on refactoring this rule to improve it and simultaneously reduce the complexity. The idea is to eventually remove the need for the generic AnalyseNodes method and have a method for specific SyntaxKinds. So I have a slight preference to add this to a separate method instead of extending the generic AnalyseNodes method.

If hope you don't mind I'll added an commit on this PR to move your improvement to a separate method: AnalyzeLengthDataType

Could you have a look at this and provide feedback if you're okay with these changes?

ans-bar-bm commented 1 month ago

Sure i don't mind, it does make sense to split it up. I've had a quick test, but it seems AnalyzeLengthDataType is never triggered for me. Unfortunatly i don't really know why that could be. Here's my test snippet:

table 50100 MyTable
{
    fields
    {
        field(1; Test; TeXT[10]) { }
    }

    var
        x: TeXT[100];
        y: TeXT;
}

Currently i only get a diagnostic on the y variable (and not on x and the Test field), and that one seems to come from AnalyseNodes as skipping with continue doesn't seem to work. At this point i am kind of doubting myself tho since i did a very simple IsKind(...) check while debugging at that also failed despite the debugger showing me the right SyntaxKind... Maybe you could check if the test works for you?

Arthurvdv commented 1 month ago

I've did some more testing, unfortunately I can't seem to reproduce the same results as you are experiencing. The sample you provided all three instances get's flagged by the 0005 rule it seems. Currently I have no idea why this isn't working for you.

image

ans-bar-bm commented 1 month ago

It's very likely i have just screwed something up on my end then. Thanks for taking a look / refactoring the changes; let me know if there is anything else left for me to do.

Arthurvdv commented 1 month ago

@ans-bar-bm No problem! Again thank you for proving the PR, this was of a great help to determine the logic to prevent the duplicate diagnostic.