From: "João Távora" <joaotavora@gmail.com>
To: Alan Mackenzie <acm@muc.de>, Beatrix Klebe <beeuhtricks@gmail.com>
Cc: emacs-devel <emacs-devel@gnu.org>
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 [thread overview]
Message-ID: <CALDnm50pBD5e4Yi+9cT=R+0tAwKyqPHGgxGKxymGFoobtYFLLw@mail.gmail.com> (raw)
In-Reply-To: <20190117164350.GA18314@ACM>
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.
> 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
next prev parent reply other threads:[~2019-01-17 17:57 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-17 14:57 Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it João Távora
2019-01-17 16:43 ` Alan Mackenzie
2019-01-17 17:57 ` João Távora [this message]
2019-01-17 18:55 ` João Távora
2019-01-18 17:54 ` Alan Mackenzie
2019-01-18 19:55 ` João Távora
2019-01-18 22:53 ` Alan Mackenzie
2019-01-19 3:18 ` João Távora
2019-01-19 11:07 ` Alan Mackenzie
2019-01-19 13:52 ` João Távora
2019-01-19 17:45 ` Alan Mackenzie
2019-01-19 19:15 ` João Távora
2019-01-19 20:58 ` Alan Mackenzie
2019-01-19 23:18 ` João Távora
2019-01-21 18:04 ` Stefan Monnier
2019-01-21 20:45 ` Alan Mackenzie
2019-01-21 21:46 ` Stefan Monnier
2019-01-21 22:41 ` João Távora
2019-01-19 22:37 ` Drew Adams
2019-01-20 19:05 ` Richard Stallman
2019-01-20 21:18 ` João Távora
2019-01-21 20:59 ` Richard Stallman
2019-01-21 21:49 ` João Távora
2019-01-18 21:06 ` Stefan Monnier
2019-01-19 3:25 ` João Távora
2019-01-18 22:47 ` Michael Albinus
2019-01-19 20:18 ` Richard Stallman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CALDnm50pBD5e4Yi+9cT=R+0tAwKyqPHGgxGKxymGFoobtYFLLw@mail.gmail.com' \
--to=joaotavora@gmail.com \
--cc=acm@muc.de \
--cc=beeuhtricks@gmail.com \
--cc=emacs-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.