From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.devel Subject: Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it. Date: Fri, 18 Jan 2019 17:54:37 +0000 Message-ID: <20190118175437.GA4095@ACM> References: <20190117164350.GA18314@ACM> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1547834641 6139 195.159.176.226 (18 Jan 2019 18:04:01 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 18 Jan 2019 18:04:01 +0000 (UTC) User-Agent: Mutt/1.10.1 (2018-07-13) Cc: emacs-devel To: =?iso-8859-1?Q?Jo=E3o_T=E1vora?= Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Jan 18 19:03:57 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1gkYUm-0001QO-8l for ged-emacs-devel@m.gmane.org; Fri, 18 Jan 2019 19:03:56 +0100 Original-Received: from localhost ([127.0.0.1]:44782 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gkYWt-0004ZA-61 for ged-emacs-devel@m.gmane.org; Fri, 18 Jan 2019 13:06:07 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:37476) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gkYWj-0004YT-HB for emacs-devel@gnu.org; Fri, 18 Jan 2019 13:05:58 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gkYWf-0003mx-Vz for emacs-devel@gnu.org; Fri, 18 Jan 2019 13:05:55 -0500 Original-Received: from colin.muc.de ([193.149.48.1]:14328 helo=mail.muc.de) by eggs.gnu.org with smtp (Exim 4.71) (envelope-from ) id 1gkYWf-0003hn-5G for emacs-devel@gnu.org; Fri, 18 Jan 2019 13:05:53 -0500 Original-Received: (qmail 70627 invoked by uid 3782); 18 Jan 2019 18:05:49 -0000 Original-Received: from acm.muc.de (p4FE15D9A.dip0.t-ipconnect.de [79.225.93.154]) by colin.muc.de (tmda-ofmipd) with ESMTP; Fri, 18 Jan 2019 19:05:48 +0100 Original-Received: (qmail 7574 invoked by uid 1000); 18 Jan 2019 17:54:37 -0000 Content-Disposition: inline In-Reply-To: X-Delivery-Agent: TMDA/1.1.12 (Macallan) X-Primary-Address: acm@muc.de X-detected-operating-system: by eggs.gnu.org: FreeBSD 9.x [fuzzy] X-Received-From: 193.149.48.1 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:232457 Archived-At: 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 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).