Highlight partially quoted arguments correctly #454
Conversation
phy1729
force-pushed
the
arg-quoting
branch
2 times, most recently
from
f1b6d52 to
892a93d
Compare
docs/highlighters/main.md
Outdated
| @@ -38,6 +38,7 @@ This highlighter defines the following styles: | |||
| * `single-quoted-argument` - single quoted arguments (`` 'foo' ``) | |||
| * `double-quoted-argument` - double quoted arguments (`` "foo" ``) | |||
| * `dollar-quoted-argument` - dollar quoted arguments (`` $'foo' ``) | |||
| * `rc-quote` - two single quotes inside single quotes when `RC_QUOTES` is set (`` 'foo''bar' ``) | |||
There was a problem hiding this comment.
RC_QUOTES → "the RC_QUOTES option" ?
| @@ -103,6 +104,12 @@ _zsh_highlight_main_add_region_highlight() { | |||
| _zsh_highlight_add_highlight $start $end "$@" | |||
| } | |||
|
|
|||
| _zsh_highlight_main_add_region_highlights() { | |||
There was a problem hiding this comment.
Could we make this function's name less similar to _zsh_highlight_main_add_region_highlight?
There was a problem hiding this comment.
s/add/add_many/
I assume the long-term plan is to make the "add many" function (however named) be more efficient than repeated calls to the "add one" function, yes?
There was a problem hiding this comment.
Yes I was thinking it could tie in nicely with #388 .
| "8 10 default" # bar | ||
| "11 14 double-quoted-argument" #"baz | ||
| "15 19 dollar-double-quoted-argument" # $quux | ||
| "20 23 double-quoted-argument" # foo |
There was a problem hiding this comment.
This is very minor, but: foo -> /foo
| { | ||
| local i | ||
|
|
||
| _zsh_highlight_main_add_region_highlight $start_pos $end_pos default |
There was a problem hiding this comment.
Should already_added=1 be here, rather than in the caller? To avoid action at a distance. Or we could just document in a comment that this function always adds, so if this is changed we'll have a reminder to update the caller.
There was a problem hiding this comment.
I think I'd prefer documenting it always add default. I also want to see if I can remove already_added, so I'd prefer to keep it in one place.
| _zsh_highlight_main_add_region_highlight $start_pos $end_pos globbing | ||
| break | ||
| fi;; | ||
| *) continue;; |
There was a problem hiding this comment.
A potential enhancement (for a separate PR :)) would be to also find {brace,expansions} and [character_classes] here. (And maybe more exotic forms of globbing, such as foo(bar|baz)qux, as well.)
| done | ||
| } | ||
|
|
||
| # Highlight single-quoted strings |
There was a problem hiding this comment.
Could you document the function's contract (input parameters, return value, use of REPLY if any)?
|
|
||
| if [[ $useroptions[rcquotes] == on ]]; then | ||
| while [[ $arg[i+1] == "'" ]]; do | ||
| highlights+=($(( start_pos + i - 1 )) $(( start_pos + ++i )) rc-quote) |
There was a problem hiding this comment.
Parentheses around ++i? Better yet, break incrementing i out to a separate line, since it's not obviously correct to have ++i on the same line as i - 1?
| done | ||
| fi | ||
|
|
||
| highlights+=($(( start_pos + $1 - 1 )) $(( start_pos + i )) single-quoted-argument $highlights) |
There was a problem hiding this comment.
I'm a bit surprised that this works even though rc-quotes is added to $region_highlight before single-quoted-argument.
|
Let's please check the performance implications of this: iterating characterwise can be slow. |
phy1729
force-pushed
the
arg-quoting
branch
2 times, most recently
from
314e75d to
48b00c6
Compare
Closes #130