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

An edge case missed by pytmc pragmalint #323

Open
ZLLentz opened this issue Aug 24, 2023 · 5 comments
Open

An edge case missed by pytmc pragmalint #323

ZLLentz opened this issue Aug 24, 2023 · 5 comments

Comments

@ZLLentz
Copy link
Member

ZLLentz commented Aug 24, 2023

In this example, we miss the malformed pragma because it is so malformed that pytmc doesn't even register it as a pragma.
I'm not sure how wide the scope of pragmalint should be but I thought I'd note this here.

https://github.com/pcdshub/lcls-twincat-general/blob/73cdd56e72eea0bb6fcd871b207bf4f621023a57/LCLSGeneral/LCLSGeneral/DUTs/DUT_EPS.TcDUT#L31

    {atribute 'pytmc' := '
        pv: bEPS_OK
        io: i
        field: DESC check if nFlags are all true;
    '}
    bEPS_OK : BOOL := TRUE;
 {atribute 'pytmc' := '
atribute
@ZLLentz
Copy link
Member Author

ZLLentz commented Aug 24, 2023

I think the ; is also invalid here, but it isn't noticed because this isn't an attribute pragma

@klauer
Copy link
Contributor

klauer commented Aug 24, 2023

; is actually valid (see #306 )

Our regex looks like this:

PRAGMA_RE = re.compile(
r"^{\s*attribute[ \t]+'pytmc'[ \t]*:=[ \t]*'(?P<setting>[\s\S]*)'}$", re.MULTILINE
)

So it doesn't consider "oops they might have typed attribute wrong"

I'm up for suggestions as to how to better restructure that - just it's tough to say "oh this looks kinda like a pragma"

@ZLLentz
Copy link
Member Author

ZLLentz commented Aug 24, 2023

I totally agree that this is tough- I made the issue fully knowing that we can't cover for every single kind of malformed entry, and if we try then we'll start flagging false positives of things that were never intended to be pytmc pragmas.

No immediate "clearly good" ideas- maybe we can have a separate regex that's just "includes pytmc within curly brackets but doesn't get matched here"? But of course we still miss typos of pytmc and, again, if you go too far you'll find that you can no longer include the word "pytmc" in other pragmas without linter errors.

"includes the word pytmc and has a pragma type that isn't on the list of existing pragma types" is a bit of an annoying mouthful.
"includes an invalid pragma type" is kind of reasonable but still annoying.

@klauer
Copy link
Contributor

klauer commented Aug 24, 2023

Well-stated @ZLLentz ...

For pattern matching without being excessive (I share the same concern about taking it too far and overlapping with other uses of non-pytmc pragmas), maybe checking to see if the following fail to parse as pragmas could be good enough:

  • {at.*pytmc.*}
  • {at.*pv: .*}

@ZLLentz
Copy link
Member Author

ZLLentz commented Aug 24, 2023

I need more time to think about the implications of doing those precise checks.

I think our overall goal should be to handle these two cases:

  • "attribute" is misspelled in a way that is non-obvious to the eye
  • "pytmc" is misspelled in a way that is non-obvious to the eye

To me, misspellings are most likely to be non-obvious when the first (and to a lesser extent, the last) letters are correct but there's some mix-up in the middle.

So maybe something similar to your suggestions are all that's needed to hit these sorts of cases, without reaching too far into collateral damage areas.

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

No branches or pull requests

2 participants