-
Notifications
You must be signed in to change notification settings - Fork 1.4k
main: Do not highlight options after -- #502
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 implementing this, and sorry for the very late review.
The test LGTM 👍. While reviewing the implementation found a minor issue which appears to be preëxisting, and filed it as #577.
|
||
# The Great Fork: is this a command word? Is this a non-command word? | ||
if [[ -n ${(M)ZSH_HIGHLIGHT_TOKENS_COMMANDSEPARATOR:#"$arg"} ]]; then | ||
seen_dashdash=0 |
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 think the variable may need to be reset to 0 in a few more cases: consider { cat -n -- -n } always { printf %s -n }
and : > >(ls)
, both of which start a new simple command without using ZSH_HIGHLIGHT_TOKENS_COMMANDSEPARATOR tokens. That said, we seem not to get this correctly for other cases, either: raised that as #577.
Even more obscure: svnadmin freeze -- /path/to/foo/ ls -l
. That -l
is an option to ls
, but in svnadmin
the --
is required. Admittedly, most commands that execv() their positional arguments don't require a --
(for example, env -i /bin/ls -l
), so maybe we should just accept this.
(For the curious, yes, that nests. svnadmin freeze -- foo/ svnadmin freeze -- bar/ ls -l
is valid.)
@phy1729 This is milestoned 0.8.0. Do you intend to work on it for the 0.8.0 release? If yes, great; if not, could you re-milestone it appropriately? Cheers. |
(Assigning until the question is answered. Feel free to unassign yourself if needed.) |
Triage: Bumping to the next milestone due to feedback timeout. |
Re #491, something like this? It does incorrectly highlight
-n
as path in a case statement like(Should be
single-hyphen-option
; is highlighted aspath
.)