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

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

Merged

Conversation

ans-bar-bm
Copy link
Contributor

Fixes #248
Fixes #639

  • Avoids duplicate diagnostics from LengthDataType and SimpleTypeReference when no range expression is set (Text with unlimited length).
  • Adds diagnostic when LengthDataType range expression is set and no SimpleTypeReference parent syntax node is present (table field definitions).

@Arthurvdv
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

@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.

@Arthurvdv Arthurvdv changed the base branch from master to prerelease June 10, 2024 14:56
@Arthurvdv Arthurvdv merged commit 3d8a998 into StefanMaron:prerelease Jun 10, 2024
35 checks passed
@ans-bar-bm ans-bar-bm deleted the feature/LC0005LengthDataType branch July 30, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LC0005 Duplicate diagnostic for unlimited texts LC0005 field datatype with length
2 participants