Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it.

[Alan Mackenzie <acm@muc.de>]

Hello, João.

First of all, I will say that I take exception, great exception, to your
means of expressing yourself, both in your post and (even more so) in
the commit you made.  It verges on sarcasm, and is totally over the top
and uncalled for.

I will say no more about this from now on.

On Thu, Jan 17, 2019 at 17:57:24 +0000, João Távora wrote:
> Hello again, Alan

> On Thu, Jan 17, 2019 at 4:54 PM Alan Mackenzie <acm@muc.de> wrote:

> > >     Temporarily comment out CC Mode from tests which are incompatible
> > > with it.

> > That would leave lots of failed tests in make check.  People have
> > already remarked on those failures, implicitly asking me to fix them.

> Yes. But the correct fix was to revert the commit that broke
> the tests, not this volkswagenesque atrocity.

The tests are now broken.  They make assumptions about CC Mode's workings
which don't hold.  Why can't we cooperate to fix them?

> > My understanding, from a previous encounter, was that having no failing
> > unit tests was of a high priority.  I've only commented a little bit
> > out, I haven't made any permanent, unreverseable changes.

> Then you'll certainly be OK if I revert your two commits myself:

I am not at all OK about this.

> the commit that broke the tests and the commit that disables the test.
> That is certainly not permanent or unreversable, too!

No, but the amount of work it now requires is greater that immediately
after my commit.

> > With that test in, I got the error message: "No test named
> > `electric-pair-whitespace-chomping-2-at-point-4-in-c++-mode-in-strings'",
> > and no other tests were performed, leaving an electric-tests.log file 86
> > bytes long.  That's why I commented it out.  This may be some glitch in
> > the testing system.

> I see, you disabled all c++ tests, *sigh*

Yes.  I couldn't understand the (undocumented) structure of these tests
well enough to make better targetted amendments.

> > > C Mode doesn't use electric-layout mode, but a user can surely
> > > decide he wants to use it in c-mode, can he not??

> > No.  Certainly not at the moment.

> No? Will you come to his house and tell him personally not to
> M-x electric-layout-mode? What if that's what he _prefers_?

That user is purely hypothetical.  And if there actually were such a
user, I would urge him to use CC Mode's auto-newline mode, which is much
better tailored to CC Mode than a generic function could be.

And if he insisted, then maybe we could look at adding
electric-layout-mode support to CC Mode.  But it would be a lot of work
for little return, given that auto-newline mode exists, works, and works
well.

> > > These tests pass fine currently.

> > When I ran them, there were lots of failures, because the tests assumed
> > electric-layout-mode was active.

> The tests should activate electric-layout-mode temporarily if needed and
> the revert to the previous situation.  If they aren't doing this, that is what
> needs to be fixed, but it works for me and I don't have electric-layout-mode,
> too.

electric-layout-mode doesn't, and can't work with CC Mode, in particular
with the c-electric-.... functions, because these are incompatible with a
non-nil post-self-insert-hook.

> > > Please revert this fix and lets discuss why you need to disable tests.

> > It's not a fix, it's a temporary workaround.

> With all due respect, I've heard that one before many many times.

> Do you realize that you've broken many many things about
> electric-pair-mode?

No, I don't.  It would have been good if you could have supplied me with
specific recipes to reproduce where it (electric-pair-mode) goes wrong,
thus allowing me to fix them.

> For example Beatrix, and me and everyone else will now lose
> the default "autobalancing" functionality of electric-pair-mode,
> which is its best feature! Autobalance used to work identically in
> most modes, but now you broke that. Is it worth breaking so
> many things just for fixing a much smaller case?

> Just one example of one of the many cases you broke.

What is "autobalancing" (the term doesn't appear in elec-pair.el) and how
is it broken?  Why can't you cite a recipe to reproduce the alleged
breakage?

> electric-pair-mixed-paren-3-at-point-4-in-c++-mode is a test defined
> in `electric-tests.elc'.

> Electricity test in a `c++-mode' buffer.

> Start with point at 4 in a 7-char-long buffer
> like this one:

>   |  (])  |   (buffer start and end are denoted by `|')

> Now call this:

> #[nil "\300\301!\207"
>       [electric-pair-mode 1]
>       2]

Pardon?  Do you mean M-: (electric-pair-mode 1)?

> Now press the key for: (

Done, having enabled e-p-m via M-:, and toggled CC Mode's electric-mode.

> The buffer's contents should become:

>   |  (()])  |

> , and point should be at 5.

Indeed, this is what I observe.  Forgive me for not hand typing in a
compiled binary.

> [back]

> You turn this into (())]) so now I have two unbalanced parenthesis types
> in my buffer.

Are you sure?  Wherein then, lies the difference between your compiled
binary above and M-: (electric-pair-mode 1)?  Could this be a bug in
e-p-m, or the byte compiler?

> > Anyhow, surely the implementation of Emacs should not be constrained by
> > its tests?  Rather the tests should test the chosen implementation.

> Really Alan, you completely broke electric-pair-mode by trying to
> reimplement it in cc-mode.el where it was working perfectly but for a small
> glitch (which really isn't its fault).

I think you are exaggerating somewhat.  And you'll find I didn't
"reimplement" electric-pair-mode, even though I was talking about this at
one stage.  CC Mode calls electric-pair-post-self-insert-function, but in
a controlled fashion at suitable places.

And I disagree about your "small glitch" characterisation.  Adding
post-self-insert-hook to Emacs broke, fundamentally, c-electric-...  This
was the breakage that Beatrix Klebe's bug uncovered.  My patch fixed it.

> If you're going to reimplement it, reimplement *all* of it (of course you might
> come to the conclusion that its best to use the existing implementation...)

I have used the existing implementation.

> > > If we come to the conclusion that some tests are asserting unreasonable
> > > expectations about the functionality you develop, we can disable them on a
> > > case by case basis!

> > I would have done that, indeed tried to do that, but the lack of
> > documentation of electric-pair-test-for, electric-pair-define-test-form
> > and so on, many of them with 8, 9 or more parameters, made that too
> > difficult, given the urgency I felt to produce a workaround.

> If this is indeed urgent, you can add some conditional check to the
> offending code using 'ert-running-test'.

Maybe you could do this.  I'm not familiar enough with the code in
electric-test.el, important pieces of which, I repeat myself, are wholly
undocumented.

> > > If on the other hand, if you need to do some work "temporarily", then
> > > the best way to do it without disturbing other people's developments
> > > is to do it in a separate branch.

> > I fixed bug #33794[*] on master, as I always do with CC Mode bugs (and most
> > other sorts of bugs, too).  That fix is, in principle, sound.  I didn't
> > discover the problems with the unit tests till later (though I admit I
> > should have done).

> It's *not* sound if it breaks tests. At least not without first arguing
> that the tests are placing unreasonable expectations on your code.

The tests do place such unfounded expectations on the code.  In
particular, the contents of post-self-insert-hook.

> > [*] That is, Beatrix Klebe's bug about CC Mode's auto-newlines not
> > working with electric-pair-mode.

> Beatrix Klebe, can, as a workaround, use electric-layout-mode
> perfectly well for her use case (even though you insist she can't)
> Until you can sort out c++-mode.

I've sorted out CC Mode.  Beatrix Klebe does not need any workarounds,
given that the bug she reported has been properly fixed.  It is likely
she is currently using that fix.

> It's much easier than creating this mess that really interferes
> with other's peoples work.

I would ask you to consider that the mess was created by you, last night,
and it interferes greatly with my work.  This mess was an order of
magnitude greater than the mess you assert I made in electric-tests.el.

I don't consider the temporary changes I made in electric-tests.el were
unreasonable, and if you disagree you could have discussed it with me in
a less disagreeable fashion than what transpired.

And now, it seems your idea is to leave special purpose code in
CC Mode, just to obviate the need to amend electric-tests.el.  As you can
imagine, I'm not keen on that.

[ .... ]

> In the meantime. I will have to fix this differently. I will add a
> temporary variable that I can set to have sane C++ behaviour
> in the meantime. I will set this variable when running tests, so
> the test people will see correct results.

That variable must go.

I have another suggestion: you could amend electric-tests.el not to
attempt to use CC Mode at all - or stick with plainer-c-mode, possibly
writing a plainer-c++-mode.  After all, the purpose is to test the
electric-... functions, not CC mode.  That could save a lot of argument.

> João

-- 
Alan Mackenzie (Nuremberg, Germany).