From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: Flymake refactored Date: Thu, 28 Sep 2017 23:11:52 -0400 Message-ID: References: <87h8vmj3tr.fsf@lolita> <87y3oygxqb.fsf@lolita> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1506654766 2820 195.159.176.226 (29 Sep 2017 03:12:46 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 29 Sep 2017 03:12:46 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) Cc: emacs-devel@gnu.org To: joaotavora@gmail.com (=?windows-1252?B?Sm/jbyBU4XZvcmE=?=) Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Sep 29 05:12:41 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dxljB-00009q-Ib for ged-emacs-devel@m.gmane.org; Fri, 29 Sep 2017 05:12:37 +0200 Original-Received: from localhost ([::1]:33464 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dxljI-0006hV-KR for ged-emacs-devel@m.gmane.org; Thu, 28 Sep 2017 23:12:44 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:32920) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dxlie-0006hJ-2L for emacs-devel@gnu.org; Thu, 28 Sep 2017 23:12:05 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dxliZ-0006yG-VM for emacs-devel@gnu.org; Thu, 28 Sep 2017 23:12:04 -0400 Original-Received: from pmta11.teksavvy.com ([76.10.157.34]:64910) by eggs.gnu.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.71) (envelope-from ) id 1dxliZ-0006ww-N9 for emacs-devel@gnu.org; Thu, 28 Sep 2017 23:11:59 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2GbDwBLuc1Z/1V13mheHgYMg1xEWTWJL?= =?us-ascii?q?IYGjmwBgXUvAUmVQIIEhT8EAgKEJFgBAgEBAQEBAgNoKIUZAQQBViMFCwstBxI?= =?us-ascii?q?UGA2KYAipOotDAQEIAiaDK4U9ghtYNYQ/gQSFNAWYUYhXlnOJNzWHB5cGWIEOM?= =?us-ascii?q?iEIMkmFGQEcggMkiGsBAQE?= X-IPAS-Result: =?us-ascii?q?A2GbDwBLuc1Z/1V13mheHgYMg1xEWTWJLIYGjmwBgXUvAUm?= =?us-ascii?q?VQIIEhT8EAgKEJFgBAgEBAQEBAgNoKIUZAQQBViMFCwstBxIUGA2KYAipOotDA?= =?us-ascii?q?QEIAiaDK4U9ghtYNYQ/gQSFNAWYUYhXlnOJNzWHB5cGWIEOMiEIMkmFGQEcggM?= =?us-ascii?q?kiGsBAQE?= X-IronPort-AV: E=Sophos;i="5.42,451,1500955200"; d="scan'208";a="5299804" Original-Received: from 104-222-117-85.cpe.teksavvy.com (HELO pastel.home) ([104.222.117.85]) by smtp.teksavvy.com with ESMTP; 28 Sep 2017 23:11:52 -0400 Original-Received: by pastel.home (Postfix, from userid 20848) id C094762D8D; Thu, 28 Sep 2017 23:11:52 -0400 (EDT) In-Reply-To: <87y3oygxqb.fsf@lolita> (=?windows-1252?Q?=22Jo=E3o_T=E1vora?= =?windows-1252?Q?=22's?= message of "Fri, 29 Sep 2017 01:22:20 +0100") X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 76.10.157.34 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:218877 Archived-At: >> We should change checkdoc.el directly to avoid such hackish surgery. >> ... >> Similarly, we should change directly byte-compile-log-warning such that >> tnhis kind of surgery is not needed. > Sure, no problem, i was in a hurry and didn't want to touch more > files. Yes, of course I understand that. > (...but didn't you use to be the advocate for add-function > everywhere, as in add-function versus hooks? Isn't this slightly > better?) A `foo-function` variable (manipulated with add-function) or a `foo-functions` (manipulated with add-hook) is a completely different issue than (cl-letf (((symbol-function 'foo) ...)) ...). The first two mean that the code was designed specifically to allow such customization and e use the advertized API for it, whereas the second means that we opportunistically take advantage of some accidental details of the code. Also the first two can be modified buffer-locally contrary to the second which naturally has a global effect. >>> +(defun flymake-elisp-checkdoc (report-fn) >>> + "A flymake backend for `checkdoc'. >>> +Calls REPORT-FN directly." >>> + (when (derived-mode-p 'emacs-lisp-mode) >> This test should not be needed. > ..if flymake-elisp-checkdoc is always placed in the correct buffer-local > value of flymake-diagnostic-functions. Indeed. It will be installed there by elisp-mode. I see no reason to assume that hordes of users are going to come and install it on the global part of the hook. And if they do, they get what they ask for. >> This also means remove the (require 'flymake-ui) >> and instead mark `flymake-make-diagnostic` as autoloaded. > Hmmm. If I read your reasoning correctly, you want to avoid > elisp-mode.el knowing about flymake and also avoiding a compilation > warning. Not really: I just don't want to preload flymake-ui into the dumped Emacs. > But isn't having flymake-make-diagnostic and the hook already > "knowing" about flymake)? I think flymake should become part of the global infrastructure which major modes will usually want to support. Like imenu, font-lock, indent-line-function, eldoc, prettify-symbols, etc... For that, they will have to know something about flymake, of course (at the very least they'll need to write a backend function for it). That doesn't necessarily mean requiring flymake, tho, since the user may or may not use flymake (again, like imenu, font-lock, eldoc, prettify-symbols, ...). > Isn't (eval-when-compile (require > 'flymake-ui)) better? Could be, but the byte-compiler will then still complain that you're calling flymake-make-diagnostic while it doesn't know that it will be available at runtime. >> This smells of legacy: just as we do for Elisp, it should be the major >> mode which specifies how to get the diagnostics, so we don't need to >> care about filenames. >> So this should be marked as obsolete. > Sure, but you're assuming a correspondence between the cars of those and > major modes. And to be honest that really sounds like boring work. My > plan was to leave flymake-proc.el quiet in a dark corner and > flymake-proc-legacy-flymake in the global hook until we write proper > backends for a sufficient number of modes. We're in violent agreement. >> If flymake-proc.el is considered legacy that should disappear in the >> long run, then I'm not sure all that renaming is justified (we can keep >> using the old names until we get rid of them altogether). > I don't agree, I like the -proc prefix to remind me not to touch it. With all the "flymake-proc--" aliases out of the way, maybe there are sufficiently few remaining aliases that I can agree (I haven't looked at the result yet). >> Also some parts of flymake-proc seem to make sense for "all" backends >> rather than only for flymake-proc. > Sure, they should be extracted into some flymake-util.el or > flymake-common.el or just flymake.el. But they could just be rewritten. Let's try and move the ones that were sufficiently well designed that we can keep using them in flymake.el without regret. For the others, they can definitely stay in flymake-proc.el. >> Some concrete examples: >> >>> + (define-obsolete-variable-alias 'flymake-compilation-prevents-syntax-check >>> + 'flymake-proc-compilation-prevents-syntax-check "26.1" >>> + "If non-nil, don't start syntax check if compilation is running.") >> >> This doesn't look specific to the specific flymake-proc backend, so >> maybe it should stay in flymake(-ui).el > IDK, there's no real conflict AFAIK, it's just a convenience. And M-x > compile is for external processes just like flymake-proc.el. But OK. IIUC you're saying it's just not useful enough to keep it in flymake.el? >> (:constructor flymake-make-diagnostic (buffer beg end type >> text)) > Also that, but actually I kept that indirection to remind me that I > probably want to create different objects for different types of objects > in the future. But then didn't go that way. That makes sense as well. I guess the main thing for me is to try not to go through the default constructor of cl-defstruct (the one with all the key arguments), since it expands to a fairly large function, and calls to it need a somewhat costly compiler-macro to turn them into efficient code. The inefficiency is nothing to worry about, but if we don't make use of the feature, it's just pure waste. >>> +* The symbol `:progress', signalling that the backend is still >>> + working and will call REPORT-FN again in the future. >> This description leaves me wondering why a backend would ever want to >> do that. What kind of effect does it have on the UI? > Nothing for the moment. But consider in the future that the UI wants to > know if it should wait for a backend that is taking too long to > report. This could be its keepalive. Or maybe your idea of calling > REPORT-FN multiple times takes care of it. I see. Indeed, then, a value of nil would play the same role, I guess. >> I don't understand this last paragraph. AFAICT from this docstring, >> REPORT-FN will be a function which takes a single argument, so I don't >> know what "provide REPORT-FN with a string as the keyword argument >> `:explanation'" means. Does it mean something like >> (funcall REPORT-FN `(:explanation ,str)) > No it means > (funcall REPORT-FN (nth (random 2)'(:panic :progress)) :explanation "this") Ah, I see. I guess it should be rephrased to make it more clear, then. >> Regarding flymake-diagnostic-functions I've been wondering about its >> behavior for very large buffers. More specifically I've been wondering >> whether it would make sense to: >> A- provide to the backend function the BEG/END of the region changed >> since the last time we called it. >> B- make it possible to focus the work on the currently displayed part of >> the buffer. >> But I guess it's not worth the effort: most/all backends won't be able >> to make any use of that information anyway. > This makes sense. The backend is called in the buffer so it has B. Well, it doesn't really have B: it could try and compute it with get-buffer-window-list and such, but it wouldn't easily be able to take into account which parts were already covered by previous calls, etc... > I was also very much thinking of making A, by collecting regions in > flymake-after-change-function, and storing them in a buffer-local > variable, like flymake-check-start-time or flymake-last-change-time. > Or perhaps dynamically binding it around the backend call. Or just > passing them as arguments. But which kind of backend would be able to make good use of such info? There might be a few (mostly the rare few implemented in Elisp, I guess), but I'm not sure it's worth the trouble (it implies a lot of work both on flymake side and on the backend side). For several checkers a change to line L2 might cause changes to the diagnostics on lines before L2, so for many backends the only way to work is "globally". If we want to link something like nxml's checker into flymake in a good way, we'll probably just need a completely different hook than flymake-diagnostic-functions. > How would I mixin existing stuff for a supposed :my-error that is > just like :error, but overrides some properties. Not sure I understand the question: how would you do it with the flymake-diagnostic-types-alist you currently have? >> `flymake-ui.el` should be renamed flymake.el (which will solve the >> above yuckiness). Admittedly, this will cause problems with mutual >> dependencies between flymake.el and flymake-proc.el, so it'll take some >> effort to solve these issues. > Perhaps solved by making the function flymake-proc-legacy-flymake, the > variable flymake-proc-err-line-patterns, and some others autoloaded? Could be, I haven't looked at them (and don't have access to the code right now). > Already fixed: > * Scrap the M-n, M-p commit. > * Delete many obsolete aliases. > * :group in defcustom > * flymake-make-diagnostic docstring > * overlay property is just 'flymake > * cl-remove-if-not and cl-sort Great. > Will fix ASAP: > > * Move flymake-compilation-prevents-syntax-check to flymake-ui.el > * flymake.el autoload yuckiness. > * Make a flymake-category. > * Make flymake-diagnostic-functions a hook. Use run-hook-wrapped. > * Obsolete alias for flymake-mode-on/off > * Don't use hash-table-keys, unless I really need it. Sounds good. > Will fix, but harder: > * proper checkdoc.el interface > * proper byte-compile.el interface > * Move flymake-elisp.el contents to elisp-mode.el I can help with that. > * Allow REPORT-FN to be called multiple times from backends There's no hurry for that. > * Passing recent changes to backends Maybe we should keep that for later, to avoid overengineering the API. > * Rename flymake-ui.el to flymake.el and make some autoloads. That'd be good. > Have doubts: > * remove starting test in flymake-elisp-checkdoc > * Use defstruct constructor for flymake-make-diagnostic > * A replacement for &key in REPORT-FN's lambda list. None of those are really important, I was just voicing my preference. > * mark flymake-proc-err-line-patterns obsolete and add to > some other variable from across emacs progmodes. I only meant to mark it as obsolete. But if the whole of flymake-proc is considered obsolete (or close enough), then it's not even worth it. > * remove the -proc prefix from defs in the proc backend. Only when it avoids obsolete aliases. And again it's just my own preference, nothing really important. > * reconsider parts from flymake-proc into an util package. If that makes sense, yes. Stefan