tests: Directly diff expected_region_highlight against region_highlight #492

phy1729 · 2018-01-20T01:15:11Z

Still has a few TODOs

comment-followed: Should the newline ending a comment be considered a commandseparator?
redirections2: What style should >(wc) be?
vanilla-newline: Unexpected entry "0 0 unknown-token". Seems to be due to the last newline in $PREBUFFER.
vi-linewise-mode: I think that standout should be 1 15, but I'm not sure.
-unclosed styles are off by one.

danielshahaf · 2018-01-20T15:24:13Z

Would this allow #475 to be implemented?

phy1729 · 2018-01-20T15:26:57Z

It couldn't test how ZLE combines styles, but it would ensure we know if we set $region_highlight differently.

danielshahaf · 2018-01-20T15:34:25Z

It couldn't test how ZLE combines styles, but it would ensure we know if we set `$region_highlight` differently.

Good enough, I think. It'd be nice to run an interactive zsh in a zpty and perform full-stack acceptance tests, but of course that's out of scope of this PR.

phy1729 · 2018-02-07T01:30:03Z

Tests pass now. There is still one XXX left which is point one above:

comment-followed: Should the newline ending a comment be considered a commandseparator?

Ready for review.

danielshahaf · 2018-02-07T11:30:28Z

Is there any region_highlight style that affects newlines?

phy1729 · 2018-02-07T12:41:04Z

Even with ZSH_HIGHLIGHT_STYLES[commandseparator]=bg=cyan,fg=cyan,bold,standout,underline, which seems to be every type of highlighting ZLE supports, I don't see anything for

: foo
bar

Although that newline is highlighted as commandseparator. I suppose this means the issue is purely academic until #327.

danielshahaf · 2018-02-10T18:59:53Z

To answer your question, I think the newline following a comment should be a commandseparator, to maintain the invariant that the token preceding a command position (a physical one, i.e., not a synthetic one after a precommand or assignment or the keyword if) is always commandseparator.

I also agree that it's an academic concern until #327 (and as such not a blocker).

phy1729 · 2018-02-10T19:28:27Z

Filed #501 and changed the test to XFAIL. Rebased onto master.

danielshahaf · 2018-02-10T20:10:15Z

highlighters/main/main-highlighter.zsh

@@ -104,7 +104,8 @@ _zsh_highlight_main_add_region_highlight() {
  (( start -= $#PREBUFFER ))
  (( end -= $#PREBUFFER ))

-  (( end < 0 )) && return # having end<0 would be a bug
+  (( start >= end )) && { print -r -- >&2 'zsh-syntax-highlighting: BUG: _zsh_highlight_main_add_region_highlight: start >= end'; return }


Suggest to interpolate "$start" and "$end" into the error message.

(Generally, most error message should have something interpolated into them.)

danielshahaf · 2018-02-10T20:12:16Z

highlighters/main/main-highlighter.zsh

@@ -104,7 +104,8 @@ _zsh_highlight_main_add_region_highlight() {
  (( start -= $#PREBUFFER ))
  (( end -= $#PREBUFFER ))

-  (( end < 0 )) && return # having end<0 would be a bug
+  (( start >= end )) && { print -r -- >&2 'zsh-syntax-highlighting: BUG: _zsh_highlight_main_add_region_highlight: start >= end'; return }
+  (( start < 0 && end <= 0 )) && return


At this point, end <= 0 implies start < 0 so the first conjunct is redundant.

danielshahaf · 2018-02-10T20:15:30Z

highlighters/main/test-data/alias.zsh

@@ -43,7 +43,7 @@ BUFFER='x.alias2; alias1'
 # functionality is present, and skip verifying suffix-alias highlighting
 # if it isn't.
 expected_region_highlight=()
-if [[ "$(type -w x.alias2)" == *suffix* ]]; then
+if zmodload -e zsh/parameter || [[ "$(type -w x.alias2)" == *suffix* ]]; then


danielshahaf · 2018-02-10T20:25:32Z

tests/test-highlighting.zsh

  done
+
+  if (( $#expected_region_highlight == $#region_highlight )); then
+    print -r -- "ok $i - cardnality check"


"cardinality" is misspelled.

danielshahaf · 2018-02-10T20:30:40Z

highlighters/main/test-data/double-quoted2.zsh

-  "3 6 double-quoted-argument-unclosed" # "foo
+  "1 1 builtin" # :
+  "3 10 default" # "foo$bar
+  "3 10 double-quoted-argument-unclosed" # "foo$bar


For reference, this double highlighting is the subject of #495 or #481.

danielshahaf · 2018-02-10T20:34:09Z

tests/test-highlighting.zsh

-  done
+  if (( unsorted )); then
+    region_highlight=("${(@n)region_highlight}")
+    expected_region_highlight=("${(@n)expected_region_highlight}")


Suppose the input contains both 42 foo and 42 bar, is numeric sorting guaranteed to always output them in the same order, regardless of what other elements the input contains?

Plain old lexicographic sorting would definitely have this property but I suspect it would make for less nice failure modes (I presume we'd want failure to be reported on the "first" non-match, for some notion of "match" that depends on the start,stop indices).

% a=(1 3 '2 foo' '2bar' '2 2 bar' '2 14 bar' '21 foo'); printf '<%s>' ${(@n)a} <1><2 2 bar><2 14 bar><2 foo><2bar><3><21 foo>

So it looks like numeric sorting falls back to lexicographic sorting.

The implementation of the (n) sorting flag (NUMERIC_GLOB_SORT option) is:

https://github.com/zsh-users/zsh/blob/567733906210feaebce94675985eb1e77602bfd8/Src/sort.c#L119-L143

In English:

Skip characters that are equal.

Store the comparison between the two different chars (This is how (n) falls back to lexicographical).

Assuming one of the differing characters is a digit, rewind to the start of digits.

Skip 0s.

Skip equal digits.

Store the difference.

Find length and return in favor of the longer or the stored difference if equal.

So I think (n) DTRT.

danielshahaf · 2018-02-10T20:37:38Z

highlighters/main/main-highlighter.zsh

@@ -888,6 +888,8 @@ _zsh_highlight_main_highlighter_highlight_single_quote()
  if [[ $arg[i] == "'" ]]; then
    style=single-quoted-argument
  else
+    # If unclosed, i points past the end
+    (( i-- ))


If I revert this line, multiline-string2 fails. 👍

danielshahaf · 2018-02-10T20:41:51Z

Thank you for making this happen!

I've went through the diff and added a few comments. I haven't the brainwidth to review all the new logic around byte/character offsets, though; I'm going to go ahead and trust you on it.

phy1729 added Improvement component:tests labels Jan 20, 2018

danielshahaf assigned phy1729 Jan 20, 2018

danielshahaf mentioned this pull request Jan 20, 2018

Highlight parameter expansion history modifiers #485

Open

phy1729 force-pushed the direct-test branch from d292b21 to 6af14dc Compare February 7, 2018 01:19

phy1729 added 2 commits February 10, 2018 13:24

driver: Replace eval with ${(P)

84f0507

driver: Do not highlight newlines or past BUFFER for zle standout

ff85c7e

phy1729 force-pushed the direct-test branch from 6af14dc to c0f8aa7 Compare February 10, 2018 19:27

danielshahaf self-requested a review February 10, 2018 20:03

danielshahaf reviewed Feb 10, 2018

View reviewed changes

phy1729 added 3 commits February 10, 2018 14:20

main: Do not highlight for regions that end before the start of BUFFER

e130745

main: Fix off by one errors in quote helper functions

fdca2ef

main: Test suffix aliases if zsh/parameter is available

622358f

phy1729 force-pushed the direct-test branch from c0f8aa7 to fa40302 Compare February 10, 2018 20:20

danielshahaf reviewed Feb 10, 2018

View reviewed changes

tests: Directly diff expected_region_highlight against region_highlight

a9be097

phy1729 force-pushed the direct-test branch from fa40302 to a9be097 Compare February 10, 2018 20:49

phy1729 merged commit a9be097 into zsh-users:master Feb 11, 2018

phy1729 deleted the direct-test branch February 11, 2018 00:49

phy1729 added this to the 0.7.0 milestone Feb 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Directly diff expected_region_highlight against region_highlight #492

tests: Directly diff expected_region_highlight against region_highlight #492

phy1729 commented Jan 20, 2018 •

edited

Loading

danielshahaf commented Jan 20, 2018

phy1729 commented Jan 20, 2018

danielshahaf commented Jan 20, 2018 via email

phy1729 commented Feb 7, 2018

danielshahaf commented Feb 7, 2018 •

edited

Loading

phy1729 commented Feb 7, 2018

danielshahaf commented Feb 10, 2018

phy1729 commented Feb 10, 2018

danielshahaf Feb 10, 2018

danielshahaf Feb 10, 2018

danielshahaf Feb 10, 2018

danielshahaf Feb 10, 2018

danielshahaf Feb 10, 2018 •

edited

Loading

danielshahaf Feb 10, 2018

phy1729 Feb 10, 2018

danielshahaf Feb 10, 2018

phy1729 Feb 10, 2018

danielshahaf Feb 10, 2018

danielshahaf commented Feb 10, 2018

tests: Directly diff expected_region_highlight against region_highlight #492

tests: Directly diff expected_region_highlight against region_highlight #492

Conversation

phy1729 commented Jan 20, 2018 • edited Loading

danielshahaf commented Jan 20, 2018

phy1729 commented Jan 20, 2018

danielshahaf commented Jan 20, 2018 via email

phy1729 commented Feb 7, 2018

danielshahaf commented Feb 7, 2018 • edited Loading

phy1729 commented Feb 7, 2018

danielshahaf commented Feb 10, 2018

phy1729 commented Feb 10, 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

danielshahaf Feb 10, 2018 • edited Loading

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 Feb 10, 2018

phy1729 commented Jan 20, 2018 •

edited

Loading

danielshahaf commented Feb 7, 2018 •

edited

Loading

danielshahaf Feb 10, 2018 •

edited

Loading