From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: joaotavora@gmail.com (=?utf-8?B?Sm/Do28gVMOhdm9yYQ==?=) Newsgroups: gmane.emacs.devel Subject: Re: Flymake refactored Date: Sun, 01 Oct 2017 17:52:23 +0100 Message-ID: <87infyizeg.fsf@lolita> References: <87h8vmj3tr.fsf@lolita> <87y3oygxqb.fsf@lolita> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Trace: blaine.gmane.org 1506876762 30129 195.159.176.226 (1 Oct 2017 16:52:42 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 1 Oct 2017 16:52:42 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Oct 01 18:52:38 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 1dyhTq-0007Ac-BZ for ged-emacs-devel@m.gmane.org; Sun, 01 Oct 2017 18:52:38 +0200 Original-Received: from localhost ([::1]:49220 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dyhTv-0001dw-TU for ged-emacs-devel@m.gmane.org; Sun, 01 Oct 2017 12:52:43 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:36499) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dyhTm-0001cV-8z for emacs-devel@gnu.org; Sun, 01 Oct 2017 12:52:35 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dyhTi-0001K7-Up for emacs-devel@gnu.org; Sun, 01 Oct 2017 12:52:34 -0400 Original-Received: from mail-wm0-x229.google.com ([2a00:1450:400c:c09::229]:53700) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dyhTi-0001Id-Jv for emacs-devel@gnu.org; Sun, 01 Oct 2017 12:52:30 -0400 Original-Received: by mail-wm0-x229.google.com with SMTP id q132so6620729wmd.2 for ; Sun, 01 Oct 2017 09:52:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:message-id:user-agent :mime-version:content-transfer-encoding; bh=T/pKk6ddQkhcsMcGKsfjQWR7QuyDcnjti/bTpiJL/74=; b=S4Ldr2F4piMmv5YhQhs7xA67IF8EUp96ootY8iIr11Tm0Cb+dALKgwazo8oLKsd8FS 7D4kHKrQWFPFwWVaKzaLZm45l+l0PpN6teupK06Mr8Y1vaA3dDvpzGkA61TNVorUTDCt VIqWAMbWI8mht5hlk5r4GtJdWOURu7GU+kaGe3moLvvbEBhKynIMm9O+qUNjCFV+KrVM 7usYzOGLSNOWWJdEO/arWQnZYLq2plBASxanjj7H+pwfHTymJVIfaQbiRIRm5txeypU9 cnxGd/ocn/kLOVbyVFKM02284yJzSw6fdv5YT/2hkoEKFOG+xnzOEJgnsOcJmOt0cgYL HI/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:message-id :user-agent:mime-version:content-transfer-encoding; bh=T/pKk6ddQkhcsMcGKsfjQWR7QuyDcnjti/bTpiJL/74=; b=F22O7EyOKRzW2ZCzwd6icXxz+Y+YGqcb269LpBsv/YLO5xOS273LHlVTkZVVIF6Hmi aRsnzuocIq+AKVQWIEcN3gGLtwvOtrBb6IAeaQNqfnXhuXZagMWfdKnYeOdytgGSKWY2 nKOySDYj134snlYx2+RKb9UoIqC5N16V9W85Iv7Hm28vTQizs63qZrM+oc7LdfrTe3uc l2vVIymDW2b/KMnKxzq6zC5aYAl9yFiHQjqta+1YngEGtDT+LWxWqI4/HLZ3Ahxp3a2z Mbxmfdn2034NyMgiqgxuqz1RbxWlAZN+hYXzAjT5x7MxT5aUCi5yys3hDTrjSHrE1TqW 21iQ== X-Gm-Message-State: AMCzsaUxcX3/vePFob5FSIO27Cvz9Nwl8FfMZhsgWEKTYQWQT+bmAO3L a1iJhsfJ9XZPfb3EhHXMpXuIjFnr X-Google-Smtp-Source: AOwi7QCOnw+m8hwaRAEaS17rWu2kSCsXmq44rDpbfXXPsjTKXa2TtO0KGcfdCdnRcN0rgrMFbbqOdw== X-Received: by 10.28.169.21 with SMTP id s21mr4913968wme.87.1506876747580; Sun, 01 Oct 2017 09:52:27 -0700 (PDT) Original-Received: from lolita.yourcompany.com (174.197.108.93.rev.vodafone.pt. [93.108.197.174]) by smtp.gmail.com with ESMTPSA id g93sm1062231wrd.87.2017.10.01.09.52.25 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 01 Oct 2017 09:52:26 -0700 (PDT) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2a00:1450:400c:c09::229 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:218989 Archived-At: [In the meantime I see you've been checking out emacs-diffs. Just a heads up that deleted the scratch/flymake-refactor and repushed it again with some different commits (but most of original tree is still identical hash-wise) Sorry for the git no-no. I did nothing special, just a bit obsessed with clean Git history I guess] So here's my reply to your email before that. Stefan Monnier writes: > 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) ...)) ...). Sorry, I mistook advice-add for add-function. I meant to say that cl-letf is better than advice-add for dynamic localized overrides, but you're right, it's not, as it binds the symbol globally. I fixed this. > 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. Of course. The probability is small, but the damage or confusion might be large, like elisp-byte-compiling an c-buffer. I made these functions error, as good practice. > Not really: I just don't want to preload flymake-ui into the dumped > Emacs. I see, because elisp-mode.el is preloaded (I was missing that bit). > 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, ...). Right, I agree. Perhaps I use autoloads sparingly (vs require) because in third-party extensions you can't generally count on autoloads across Emacs versions. >>> 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. I can only think of porting flymake-proc-stop-all-syntax-checks, but: * That is hard to do generically (requires more API) * I don't see why it's particularly useful I'd rather fix bugs in flymake-proc.el like the one that leaves those @#$% *_flymake.c files behind (I think this happens because the cleanup functions are local to a buffer that is outlived by the process, BTW) >>>> + 'flymake-proc-compilation-prevents-syntax-check "26.1" > IIUC you're saying it's just not useful enough to keep it in > flymake.el? Yes, what are we really preventing? I can only think of protecting against a Makefile being confused by, say, files of _flymake.c suffix, in the make directory. But this is a flymake-proc.el specific problem. (but this one is simpler to port anyway). > 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. I see. Let's leave it for later. > I see. Indeed, then, a value of nil would play the same role, I > guess. It does now, but before that only one call to REPORT-FN are allowed. See the docstring again and the commit where I significantly redid the API. Multiple calls to REPORT-FN are now allowed. > Ah, I see. I guess it should be rephrased to make it more clear, > then. Hopefully I did. But it probably needs your nitpicking. > 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. Really? That's disappointing... Even with the new version of 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? See the new docstring of flymake-diagnostic-types-alist: `flymake-category', a symbol whose property list is considered as a default for missing values of any other properties. This is useful to backend authors when creating new diagnostic types that differ from an existing type by only a few properties. So if elisp-flymake-checkdoc needs to make something very similar to notes but with no text face (just the fringe bitmap), it can do ... (flymake-make-diagnostic (current-buffer) start end :checkdoc-note text) ... And then (add-to-list 'flymake-diagnostic-types-alist '(:checkdoc-note . ((flymake-category . flymake-note) (face . nil)))) And then the user can override it. >> 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). Actually, I just (require 'flymake-proc) *after* (provide 'flymake) in flymake.el. It looks pretty clean to me, does you see any drawbacks? >> * Move flymake-compilation-prevents-syntax-check to flymake-ui.el Not done, not convinced of the usefulness yet. >> * 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 Done >> * Don't use hash-table-keys, unless I really need it. Actually, I think I need hash-table-keys. > >> Will fix, but harder: >> * proper checkdoc.el interface >> * proper byte-compile.el interface >> * Move flymake-elisp.el contents to elisp-mode.el No need, I think. I did these too. > >> * Allow REPORT-FN to be called multiple times from backends No, API changes big as this one are important to get right sooner rather than later. In the mantime, I noticed that Flycheck's API is very similar this one (though not exactly the same). > >> * Passing recent changes to backends > > Maybe we should keep that for later, to avoid overengineering the API. But, this one I agree to save for later. Probably binding a dynamic variable. (but we could also pass kwargs to the backends) >> * Rename flymake-ui.el to flymake.el and make some autoloads. > > That'd be good. Did that. >> * 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. Good, because I kept them >> * 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. Didn't do this too. If we mark it obsolete, what's the "use instead" message? >> * 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. See if you like it now. >> * reconsider parts from flymake-proc into an util package. > > If that makes sense, yes. Nothing strikes me as really essential in this regard, perhaps later. Jo=C3=A3o