[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests whi
From: |
João Távora |
Subject: |
Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it. |
Date: |
Thu, 17 Jan 2019 17:57:24 +0000 |
Hello again, Alan
On Thu, Jan 17, 2019 at 4:54 PM Alan Mackenzie <address@hidden> 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.
> 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:
the commit that broke the tests and the commit that disables the test.
That is certainly not permanent or unreversable, too!
> 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*
> > 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_?
> > 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.
> > 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?
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.
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]
Now press the key for: (
The buffer's contents should become:
| (()]) |
, and point should be at 5.
[back]
You turn this into (())]) so now I have two unbalanced parenthesis types
in my buffer.
> 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).
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...)
> > 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'.
> > 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.
> [*] 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.
It's much easier than creating this mess that really interferes
with other's peoples work.
> Anyhow, apologies, and all that, but I don't want to spend any more time
> on this topic today or until tomorrow evening, since I've got an exam
> coming up tomorrow. But I promise I'll get back to you (including
> answering your other post) either late tomorrow or on Saturday.
Alright. Good luck for your exam.
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.
João
- Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., João Távora, 2019/01/17
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., Alan Mackenzie, 2019/01/17
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it.,
João Távora <=
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., João Távora, 2019/01/17
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., Alan Mackenzie, 2019/01/18
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., João Távora, 2019/01/18
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., Alan Mackenzie, 2019/01/18
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., João Távora, 2019/01/18
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., Alan Mackenzie, 2019/01/19
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., João Távora, 2019/01/19
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., Alan Mackenzie, 2019/01/19
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., João Távora, 2019/01/19
- Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it., Alan Mackenzie, 2019/01/19