-
Notifications
You must be signed in to change notification settings - Fork 53
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
pygments syntax: improve cylc lexer #2274
pygments syntax: improve cylc lexer #2274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good by eyeballs 👀 (+ 👓 in my case). Do we need some automated tests?
Automated testing for syntax files, hmmm, that might be a tricky one. The simplest thing I can think of doing would be to come up with some devilishly convoluted examples to run through each editor in turn, then perform a visual diff on before and after screenshots. We need to look into this stuff anyway for future GUI development ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a way to test by giving the lexical analyser some suite.rc
fragments and see what tokens come out on the other side, then we should try that. Otherwise, change looks OK.
For See cylc/cylc-flow#2915 for a discussion on testing approaches for syntax files. |
a77f2ce
to
9bebbd0
Compare
Have added standard reference files for testing lexers in cylc/cylc-flow#2916. PS/ Spotted an edge case with |
9bebbd0
to
5913b6a
Compare
(r'\s', Text), | ||
(r'=>', Operator), | ||
(r'[\&\|\!]', Operator), | ||
(r'[\&\|]', Operator), | ||
(r'[\(\)]', Punctuation), | ||
(r'\[', Text, 'intercycle-offset'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have noticed one omission. Dependencies on specific cycle points e.g. graph = "red[2019] => blue"
are supported as valid specification within square brackets in a graph string, where only intercycle offsets appear to be recognised by the lexer as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drat.
Turns out you can also do arithmetic (e.g. [^ + P1D]
) which I didn't know. This is going to be a little trickier to support whilst maintaining error checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The good news is the lexer is now so water-tight it picked up on several missing markers in some of the Cylc documentation code excerpts. It's nearly there 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Very sorry for the delay in re-reviewing, I didn't realise commented reviews took PRs off the GitHub 'review requests' listing.)
Works as described. Lack of associated tests justified by comment.
To note: dependencies on specific cycle points are recognised after merge of this PR, but cycle point arithmetic has not been included in the PR domain & should eventually be incorporated:
Arithmetic is fine but arbitrary whitespace is not supported. Table updated accordingly |
Oh, fair enough, I simply used the example you provided in your original comment, assuming it was valid. Good! |
Support additional features of the Cylc suite.rc language to permit use in the Cylc documentation (cylc/cylc-flow#2910).
Adds support for the following (see table in cylc/cylc-flow#2752):
Fixes: