Re: [RFC] Add xfail tests for || form of completion matchers

[Marlon Richert <marlon.richert@xxxxxxxxx>]

On Wed, Oct 13, 2021 at 8:08 AM Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Oct 12, 2021 at 8:26 AM Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > Marlon Richert wrote on Tue, Oct 12, 2021 at 15:08:46 +0300:
> > > On Mon, Oct 11, 2021 at 5:34 PM Marlon Richert <marlon.richert@xxxxxxxxx> wrote:
> > > >
> > > > The tests show how :||= matchers should behave in order to provide
> > > > completion features that cannot be implemented with :|= matchers.
> >
> > Would this be backwards compatible?

No, it would not, but that's unavoidable, since at present, the :||=
matchers don't work correctly. Please see my and Oliver's comments in
the thread of users/27228.

On the plus side, there are only two lines in the Zsh codebase where
:||= matchers are used, in _ssh and _x_color. It won't require much
work to convert those.

> In particular, with the exception of specific bug regression tests,
> all the tests using || matchers have been converted to xfails.
> Shouldn't there still be some generic tests of the (functionally
> correct subset of) the current behavior of || ?

There was exactly one non-regression test using :||= matchers,
«Documentation example using "r:[^A-Z0-9]||[A-Z0-9]=** r:|=*"», and
unfortunately, that one will no longer pass as written. However, I
will see if I can find some cases for which the current implementation
works correctly and add tests for them.

> And, do you think any
> of the regression tests would begin to fail if the xfail tests begin
> to succeed?

There are four regression tests that incorporate one or more :||=
matchers. I investigated them and this is what I found:
* Bug from workers 11081 is about the cursor jumping back and forth. I
was able to remove all three :||= matchers from the test without
breaking it.
* Bug from workers 11586 is about characters getting deleted while
inserting the "unambiguous" substring. Here, I was not able to remove
or replace the :||= matcher and still get the same output. This one
might break, but most of the output it expects is actually not
relevant to the bug it is testing.
* Test from workers 13320 is about cursor positioning. I was able to
remove the :||= matcher from the test without breaking it.
* Second test from workers 13345 is about a character getting deleted.
I was able to replace the :||= matcher with a :|= matcher without
breaking the test.

On Tue, Oct 12, 2021 at 6:25 PM Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
>
> Thanks.  I have never found that section easy to follow.

You're not the only one. ;)

> Could you confirm that the text which the docs patch deletes or changes
> was all confirmed correct (even if perhaps unclear)?  I'm concerned
> about us possibly changing dense, accurate docs into clear, less-accurate
> docs.

The original docs were vague and ambiguous on many points, and even
self-contradictory on some.

> Case in point: The incumbent docs say that the coanchor is matched only
> against the trial completion, but the new docs say something else.  If
> that's an intentional change, it needs to be called out explicitly in
> the log message.

Yes, that's intentional. I'll add it to the commit message.

> In the man page rendering on my system, itemiz()'s bullet is vertically
> aligned with the parent item()'s text.

I can confirm that this indeed looks wrong. Do you know what I should
use to get them indented properly? Or if that's not possible, I can
add a ifzman check to format the text without bullets in the man page.
However, I would at least like to keep them in the html page, because
it helps make the text clearer.

On Wed, Oct 13, 2021 at 7:57 AM Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Oct 12, 2021 at 8:26 AM Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > Could you confirm that the text which the docs patch deletes or changes
> > was all confirmed correct (even if perhaps unclear)?
>
> For example, this part is misleading:
>
> > > +By default, characters in the string to be completed (referred to here as the
> > > +command line) map only onto identical characters in the list of matches
> [...]
> > > +missing characters are inserted only at the cursor position, if the shell
> > > +option tt(COMPLETE_IN_WORD) is set, or at the end of the command line,
>
> It's at the end of the current word, not the end of the command line.
> The old wording nearly always says "string on the command line" which
> is only somewhat better; if it's going to be completely rewritten to
> drop "string on the", the phrase "command line" should become more
> precise.  "Incomplete word" perhaps?

The use of "command line" in this fashion is from the original text;
it is used there about half of the time without the addition of
"string". However, I agree that it's ambiguous. I'm fine replacing it
with "incomplete word" (unless we come up with a better term).

> > > +corresponding tests are applied to both characters, but they are not otherwise
> > > +constrained; any matching character in one set goes with any matching character
> > > +in the other set: this is equivalent to the behaviour of ordinary character
> > > +classes.
>
> What's an "ordinary" character class?  That is, what ordinary context
> is implied?

I didn't write that paragraph; it was already present in the original
doc. I just moved it around.

> > > +xitem(tt(l:)var(anchor)tt(|)var(lpat)tt(=)var(tpat))
> > > +xitem(tt(L:)var(anchor)tt(|)var(lpat)tt(=)var(tpat))
> > > +xitem(tt(r:)var(lpat)tt(|)var(anchor)tt(=)var(tpat))
> > > +item(tt(R:)var(lpat)tt(|)var(anchor)tt(=)var(tpat))(
> > > +Let any command line substring, which is left/right-adjacent (respectively) to
> > > +a substring matching var(anchor) and which matches var(lpat), be completed to
> > > +any trial completion substring, which
> > > +startitemize()
> > > +itemiz(\
> > > +is adjacent to the same substring and which
>
> Unclear whether "the same substring" refers to "any command line
> substring" or to "a substring matching anchor".  I believe you mean
> the former (or perhaps the larger substring composed of both of the
> former)?  Best to specify.

Will do.

> I believe the rest of the explanations are correct, but it would be
> good if Oliver confirms.
>
> Did you remove the assorted other examples because there is a problem with them?

I split them into parts and moved each part directly beneath the
matcher(s) it uses. This makes the matchers easier to understand and
allows the examples to be explained with less text.