-
-
Notifications
You must be signed in to change notification settings - Fork 833
🐛 Fix highlighting of optional variadic argument metavars #1508
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
base: master
Are you sure you want to change the base?
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.
Thanks for the report, I'll have a look!
|
I was taking another look, and I'm wondering if there's a specific reason the opening and closing brackets are only matched at the beginning/end of the string. If not, it might make more sense to just remove the |
|
The dots indicate that multiple arguments are accepted (hence the |
| @pytest.mark.parametrize("input_text", ["[ARGS]", "[ARGS]..."]) | ||
| def test_metavar_highlighter(input_text: str): |
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.
This test succeeds on master with the normal [ARGS] but fails with [ARGS].... Succeeds with this PR.
svlandeg
left a comment
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.
Confirmed the bug though it was hardly noticeable on my console 🥲
To write a regression test for this, I decided to move MetavarHighlighter to the module level, I think it's nicer anyway to define it together with the other highlighters. Then we can test for the highlighting directly, which works fine without the 3 dots but indeed fails as the regex doesn't consider the possibility of seeing 3 dots before the end of the string.
As @BenjyWiener mentioned, another option would be to remove the $. It just feels that these start and end markers are put in the regex on purpose, so I think the current solution is fine.
| highlights = [ | ||
| r"^(?P<metavar_sep>(\[|<))", | ||
| r"(?P<metavar_sep>\|)", | ||
| r"(?P<metavar_sep>(\]|>))(\.\.\.)?$", |
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.
Tiangolo: notice how this code was moved, but also changed to add the optional 3 dots at the end of the regex.
Due to a mistake in one of the
metavar_sepregexes, a closing bracket followed by '...' does not get the correct highlighting in the arguments help panel.Minimum reproducible example:
Current
--helpoutput (note the closing bracket of[ARGS]...in theArgumentspanel):With this PR: