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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Marlon Richert wrote on Wed, Oct 13, 2021 at 17:20:09 +0300:
> 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.
> 

Care to give a more specific pointer?  As in, specific cases where the
incumbent documentation doesn't match the implementation?  users/27228
itself reads rather along the lines of "let's re-design the feature
retroactively".

If you want to clarify the documentation of the feature as designed,
kudos.  If you want to increase test coverage, more kudos.  If you want
to throw out the existing documentation and implementation and
reimplement things differently… that's not to be done lightly,
notwithstanding that you went the right way about proposing it (first
clarifying the status quo, then posting docs and XFail tests).

> 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.

That's not how it works.  Documented semantics are API promises that
should be presumed to be used in the wild.  Any change that may break
anybody's proverbial spacebar heating is an incompatible change and
should be treated accordingly (avoided if possible, and failing that,
clearly documented for upgraders, designed with a reasonable failure
mode for old code on new zsh, etc.).

When you give your house key to a trusted, you can always change the
lock and give the friend a new key.  However, user code isn't like
a house key.  User code is more closely analogous to public roads in
that old story about how the width of a car was basically determined by
the Romans (because cars had to be compatible with existing roads): it
exists, it can't be changed, it must be compatible with; it's a design
constraint.

> > 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.

A patch that breaks a documentation example is the archetype of an
incompatible change.  Is there no alternative to that?  Can't you add
a new syntax?  It can be as simple as «-M 'v2: …'» (that's pretty common
in standards that retrofit themselves into DNS TXT records, for instance).

> However, I will see if I can find some cases for which the current
> implementation works correctly and add tests for them.

Thanks.

> > 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.

The test isn't a unit test of :||=, where one would expect the output to
change when :||= is removed.  The test is a regression test that's
supposed to catch instances of a bug that could be reproduced only by
one person and only intermittently.  For such tests, changing their code
in any way might make them no longer test for the bug they claim to.

> * 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,

Ack.

> but most of the output it expects is actually not relevant to the bug
> it is testing.

FWIW, the reply to 11586, 11634, mentions CamelCase briefly.

> * Test from workers 13320 is about cursor positioning. I was able to
> remove the :||= matcher from the test without breaking it.

So what?  The question isn't whether users who have used :||= could have
written their code without it.  The question is whether users who have
used :||= would see a behaviour change if :||='s semantics were changed
in the manner proposed.

> * 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.
> 

Ditto.

> On Tue, Oct 12, 2021 at 6:25 PM 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)?  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.

Concrete examples, please?  users/27228 is a long thread, and the patch
itself is 500 80-character lines long.

> > 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.

Thanks, but again, that was just a case in point.  You need to identify
all such cases, or better yet, split the patch into a series of small,
reviewable changes.  That's always a good idea, and more so for changes
that are _a priori_ controversial (in this case, due to being backwards
incompatible).

> > 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?

No, sorry.  You might want to look in zman.yo and ztexi.yo to see if we
define or redefine startitem(), item(), startitemize(), or itemiz().  If
not, then it might be some issue that can be reproduced with yodl/nroff/man
alone, i.e., not an issue specific to zsh's yodl code.

> Or if that's not possible, I can add a ifzman check to format the text
> without bullets in the man page.

Or with ASCII bullets.

> However, I would at least like to keep them in the html page, because
> it helps make the text clearer.

Sure.