Highlight command and process substitution #512

phy1729 · 2018-03-28T04:15:29Z

Closes #139 and #494.

Needs naming suggestions for things with XXX, but should be otherwise ready,

phy1729 · 2018-03-28T04:25:25Z

Tests only fail for 5.0.8.

command-substitution-unclosed fails with

not ok 2 - [3,16] «foo$(echo bar» - expected (3 15 "default"), observed (3 16 "default").

and process-substitution2 with

not ok 6 - [17,27] «=(echo foo» - expected (17 26 "default"), observed (17 27 "default").

Not sure why yet.

phy1729 · 2018-04-04T01:21:11Z

Seems ${(z) in 5.0.8 splits : foo$(echo bar into : and foo$(echo bar (note the extra space at the end). Since this is a zsh bug that's already been fixed and will nearly never* effect the highlighting the user sees (since the off by one will almost always be past the end of the buffer), can we just add a condition in the tests to expect the wrong output on zsh 5.0.8?

*: Nearly never because it's possible that a user could nest an unclosed $( command substitution inside of a backtick command substitution.

danielshahaf · 2018-04-04T18:17:37Z

Seems `${(z)` in 5.0.8 splits `: foo$(echo bar` into `:` and `foo$(echo bar ` (note the extra space at the end). Since this is a zsh bug that's already been fixed and will nearly never* effect the highlighting the user sees (since the off by one will almost always be past the end of the buffer), can we just add a condition in the tests to expect the wrong output on zsh 5.0.8?

Sure. Making the condition dependent not on $ZSH_VERSION but on [[ ${${(z):-': foo$(echo bar'}[2]} == *' ' ]] (sp?) would be nice, if it's not much effort.

*: Nearly never because it's possible that a user could nest an unclosed `$(` command substitution inside of a backtick command substitution.

So... If someone nests a $() inside backticks under 5.0.8, their highlighting will be off-by-one until they type the closing paren? I think we can live with that :-) Cheers,

phy1729 · 2018-04-05T04:20:55Z

Making the condition dependent not on $ZSH_VERSION but on [[ ${${(z):-': foo$(echo bar'}[2]} == *' ' ]] (sp?) would be nice, if it's not much effort.

It wouldn't be hard, but I think it would be better to check $ZSH_VERSION to catch possible regressions in the future (hopefully before they become part of a release). The latest commit reflects this although the condition would be easy to change if you still think the other test is better.

danielshahaf · 2018-04-05T15:06:23Z

To catch regressions in ${(z)} is zsh's test suite's business, not ours. I vote for the functional test here and a regression test in zsh's test suite. Makes sense?

phy1729 · 2018-04-06T02:36:09Z

Works for me. Rebased.

danielshahaf · 2018-04-07T06:14:13Z

Ty. Still planning to review/test once I'm at my workstation. Haven't had time to breathe.

danielshahaf · 2018-04-14T07:41:33Z

Reviewed. Haven't tested (not at workstation). Kudos for the work & thanks for patience.

Does zsh have a regression test for that 5.0.8 bug?

danielshahaf · 2018-04-14T07:44:17Z

XXX -> parse_list? Using the zsh grammar meaning of "list".

phy1729 · 2018-04-14T14:53:09Z

Not quite correct since the function can highlight multiple lists. I don't think zsh has a name for one or more lists.

danielshahaf · 2018-04-14T16:12:03Z

What do you mean by "multiple lists"?

A list can contain other lists, e.g., a list that contains an if construct.

More generally, a list can be seen as a set of any shell commands whatsoever, including the complex commands below; this is implied wherever the word `list' appears in later descriptions. For example, the commands in a shell function form a special sort of list.

danielshahaf · 2018-04-14T16:15:24Z

highlighters/main/test-data/back-quoted-argument.zsh

  "20 39 default" # "is `echo equal` to"
  "20 39 double-quoted-argument" # "is `echo equal` to"
  "24 35 back-quoted-argument" # `echo equal`
+  "25 28 builtin" # echo
+  "30 34 default" # equal
  "41 55 default" # `echo 6 times 9


Should the backticks themselves be highlighted? Quotes and command/process substitutions are all highlighted. I suppose we could highlight backticks as magenta, like $(…), since both of these are subject to word splitting.

That wouldn't be consistent with single, double, and dollar quotes, but then again those are quotes and these are command substitutions.

What wouldn't be consistent? The colour (yellow v. magenta), or something else?

For the quoting styles, the quotes themselves are not given a special style. If $( and ) and backticks are, that would be inconsistent between the two; but they are different syntactical constructs. Additionally I think magenta (or any other really) highlighting all the words not otherwise highlighted in a command substitution is a bit much.

Agreed that highlighting all words inside a cmdsubst as magenta is a bit much. How about highlighting $(, ), ` as command-substitution-delimiter, defaulting to magenta, and keeping plain words inside the cmdsubst unhighlighted? We could even keep the command-substitution group matching the entire $(…) construct so people could assign to it in themes.

An orthogonal question: if we had command-substitution-{delimiter-,}$(( N++ )) styles, with $N being the nesting depth, we could have a rainbow parentheses theme. Featuritis?

I was thinking we'd consider that when coming up with zstyles for #362 . I like the delimiter idea. How about the latest two commits?

Note: : "$(: foo)" will still have foo highlighted yellow since there's the double-quoted-argument highlighting applying to the whole of the double quotes. Not sure what if anything to do about that.

Delimiters LGTM.

Re foo in : "$(: foo)", I don't think it should be yellow since the lexing rules of double-quoted strings (e.g., backslash escape sequences) don't apply there. That's not to say whether or not it blocks merging or releasing; just to say that I don't think it's useful for that foo to be yellow.

I agree, but I don't know what to do about it. In theory
: ${ZSH_HIGHLIGHT_STYLES[default]:=fg=default}
would help, but it appears to make no difference here. Additionally the yellow would still show through for path styled strings.

highlighters/main/main-highlighter.zsh

danielshahaf · 2018-04-14T16:27:08Z

Reviewed. Great work! Thanks for splitting the noop changes to their own commits.

danielshahaf · 2018-04-14T16:28:12Z

After touch foo, BUFFER='echo $(foo)' paints foo in magenta+underline, whereas I'd expect it to be highlighted as unknown-token since it's in command position.

phy1729 · 2018-04-14T16:34:47Z

After touch foo, BUFFER='echo $(foo)' paints foo in magenta+underline, whereas I'd expect it to be highlighted as unknown-token since it's in command position.

Similar for touch foo then BUFFER=foo, so I think it's a separate issue.

danielshahaf · 2018-04-14T17:15:39Z

I don't see the similarity: here, BUFFER=foo has nothing in magenta. (The filename is underlined, though 😄)

To be clear, in my example, I'd typed echo $(foo) at the prompt; I did not literally type BUFFER= at the prompt.

phy1729 · 2018-04-14T17:19:28Z

Oh, the magenta is because the whole $(foo) is highlighted as command-substitution. I was intending to convey once BUFFER=foo is highlighted as unknown-token so will echo $(foo).

danielshahaf · 2018-04-14T17:29:44Z

Ah, fair enough.

At the risk of shedding bikes, should the whole of $(foo) be highlighted in magenta, or just the delimiters ($( and ))?

danielshahaf · 2018-04-14T17:34:56Z

Oh, the magenta is because the whole $(foo) is highlighted as command-substitution. I was intending to convey once BUFFER=foo is highlighted as unknown-token so will echo $(foo).

That's #202.

phy1729 · 2018-04-21T05:33:54Z

highlighters/main/main-highlighter.zsh

                            in_array_assignment=true
                          else
                            # assignment to a scalar parameter.
                            # (For array assignments, the command doesn't start until the ")" token.)
                            next_word+=':start:'
+                            if (( i <= end_pos )); then


This condition is incorrect. Should be
(( start_pos + i <= end_pos ))
Fixed up in latest branch.

danielshahaf · 2018-04-21T16:33:36Z

I agree, but I don't know what to do about it. In theory `: ${ZSH_HIGHLIGHT_STYLES[default]:=fg=default}` would help, but it appears to make no difference here. Additionally the yellow would still show through for `path` styled strings.

Two things come to mind: 1. zle has some issues with composing multiple styles for the same offsets, e.g., I think `region_highlight=("1 2 fg=foo" "1 2 bg=bar")` doesn't results in fg=foo,bg=bar but in bg=bar. (I'm not sure about this specific example but that's the gist.) Depending on how you tested you may have run into that. That'll have to be fixed in zle. 2. We could break down the double-quoted string and only set fg=yellow for the parts outside of any inner command substitutions, e.g., in «: "foo `bar` baz"» we'd add separate ranges for the «foo » and « baz». I think that's what pws would describe as a finite amount of effort ;-).

danielshahaf · 2018-04-21T16:33:53Z

phy1729 commented on this pull request. This condition is incorrect. Should be ```(( start_pos + i <= end_pos ))``` Fixed up in latest branch.

Regression test?

No functional change.

This will allow for highlighting $( ) and similar.

This allows for callees to prepend highlights before $reply after the length of the feature (e.g. command substution) is known.

This will allow for correct path_prefix highlighting in backticks.

Fixes zsh-users#494

Prepares for next commit. No functional change.

No functional change.

This allows a caller to know if the command or process substitution is complete.

phy1729 changed the title ~~Command,process substitution~~ Highlight command and process substitution Mar 28, 2018

phy1729 force-pushed the command,process-substitution branch from cf8d151 to 76f6ac6 Compare April 5, 2018 04:17

phy1729 mentioned this pull request Apr 5, 2018

Works so slow when I try to paste long string to terminal #513

Closed

phy1729 force-pushed the command,process-substitution branch from 76f6ac6 to 74056f5 Compare April 6, 2018 02:35

phy1729 force-pushed the command,process-substitution branch from 74056f5 to 7af5ca9 Compare April 14, 2018 14:29

danielshahaf reviewed Apr 14, 2018

View reviewed changes

highlighters/main/main-highlighter.zsh Show resolved Hide resolved

phy1729 force-pushed the command,process-substitution branch from 67b33f6 to 38d7573 Compare April 14, 2018 19:24

phy1729 commented Apr 21, 2018

View reviewed changes

phy1729 force-pushed the command,process-substitution branch from 38d7573 to 3e9c7b9 Compare April 21, 2018 05:34

phy1729 added 22 commits September 30, 2018 14:35

main: Add buf_offset variable

dbce731

No functional change.

main: Reorder declarations for next commit

81e89a8

No functional change.

main: Split highlighting code into another function

ead8f8a

This will allow for highlighting $( ) and similar.

main: Move highlighting to _main_paint

ce592bd

This allows for callees to prepend highlights before $reply after the length of the feature (e.g. command substution) is known.

main: Highlight command substitutions

b751695

main: Track if $buf has the end of $BUFFER

c40c72c

This will allow for correct path_prefix highlighting in backticks.

main: Recurse into backtick command substitutions

9db3933

main: Add test for nested backticks; summon Cthulhu

3ac7d1c

main: Highlight <( ) and >( ) process substitutions

08d4401

Fixes zsh-users#494

main: Add test for embedded process substitution

beae087

main: Change conditional to a switch

73640b7

Prepares for next commit. No functional change.

main: Highlight =( ) process substitutions

c0e64fe

main: Add test for =( ) process substitution

49dbb05

main: Add test for unclosed command substitution

57e451b

main: Make _check_path's argument explicit

13d332a

No functional change.

main: Start _highlight_argument at $1

12e0133

No functional change.

main: Correct array assignment check

c06e5e9

main: Highlight the value of an assignment

932eb38

main: Add test for command substitution in assignment

8a93b85

main: Ignore zsh 5.0.8 off by one bug in tests

9616bfd

main: Support the glob_assign option

472c71c

main: Return 0 if braces_stack is empty

22839d0

This allows a caller to know if the command or process substitution is complete.

phy1729 force-pushed the command,process-substitution branch from 3e9c7b9 to f5f3bd9 Compare September 30, 2018 19:48

phy1729 and others added 2 commits September 30, 2018 16:26

main: Highlight {command,process} substitution delimiters

e86f75a

'main': Add regression test for an unclosed backtick substitution

565463c

phy1729 force-pushed the command,process-substitution branch from f5f3bd9 to 565463c Compare September 30, 2018 20:26

phy1729 merged commit 565463c into zsh-users:master Sep 30, 2018

phy1729 deleted the command,process-substitution branch September 30, 2018 20:41

phy1729 added this to the 0.7.0 milestone Oct 1, 2018

phy1729 mentioned this pull request Oct 21, 2018

Thoughts on recursive highlighting (inside backticks etc.) #389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Highlight command and process substitution #512

Highlight command and process substitution #512

phy1729 commented Mar 28, 2018 •

edited

Loading

phy1729 commented Mar 28, 2018

phy1729 commented Apr 4, 2018

danielshahaf commented Apr 4, 2018 via email

phy1729 commented Apr 5, 2018

danielshahaf commented Apr 5, 2018

phy1729 commented Apr 6, 2018

danielshahaf commented Apr 7, 2018

danielshahaf commented Apr 14, 2018

danielshahaf commented Apr 14, 2018

phy1729 commented Apr 14, 2018

danielshahaf commented Apr 14, 2018

danielshahaf Apr 14, 2018

phy1729 Apr 14, 2018

danielshahaf Apr 14, 2018

phy1729 Apr 14, 2018

danielshahaf Apr 14, 2018

phy1729 Apr 14, 2018

danielshahaf Apr 19, 2018

phy1729 Apr 20, 2018

danielshahaf commented Apr 14, 2018

danielshahaf commented Apr 14, 2018 •

edited

Loading

phy1729 commented Apr 14, 2018

danielshahaf commented Apr 14, 2018

phy1729 commented Apr 14, 2018

danielshahaf commented Apr 14, 2018

danielshahaf commented Apr 14, 2018

phy1729 Apr 21, 2018

danielshahaf commented Apr 21, 2018 via email

danielshahaf commented Apr 21, 2018 via email

Highlight command and process substitution #512

Highlight command and process substitution #512

Conversation

phy1729 commented Mar 28, 2018 • edited Loading

phy1729 commented Mar 28, 2018

phy1729 commented Apr 4, 2018

danielshahaf commented Apr 4, 2018 via email

phy1729 commented Apr 5, 2018

danielshahaf commented Apr 5, 2018

phy1729 commented Apr 6, 2018

danielshahaf commented Apr 7, 2018

danielshahaf commented Apr 14, 2018

danielshahaf commented Apr 14, 2018

phy1729 commented Apr 14, 2018

danielshahaf commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielshahaf commented Apr 14, 2018

danielshahaf commented Apr 14, 2018 • edited Loading

phy1729 commented Apr 14, 2018

danielshahaf commented Apr 14, 2018

phy1729 commented Apr 14, 2018

danielshahaf commented Apr 14, 2018

danielshahaf commented Apr 14, 2018

Choose a reason for hiding this comment

danielshahaf commented Apr 21, 2018 via email

danielshahaf commented Apr 21, 2018 via email

phy1729 commented Mar 28, 2018 •

edited

Loading

danielshahaf commented Apr 14, 2018 •

edited

Loading