Flymake refactored

Flymake refactored

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

Hi all,

I think my Flymake refactoring/rewriting effort is nearing
completion. Obviously people to take a look at it before it is
merged to master.

Here are the main changes:

* Flymake now annotates arbitrary buffer regions, not just lines.

* Flymake supports arbitrary diagnostic types, not just errors and
  warnings. See variable flymake-diagnostic-types-alist.

* There is a clean separation between the UI showing the diagnostics
  and the sources of these diagnostics, called backends.

* Flymake supports multiple simultaneous backends, meaning that you can
  check your buffer from different perspectives.

* The "legacy" regexp-based backend still works nicely in my (limited)
  testing. It is enabled by default but lives in a separate file
  lisp/progmodes/flymake-proc.el

* Two backends for Emacs-lisp are provided in
  lisp/progmodes/flymake-elisp.el. Enabling flymake-mode in an elisp
  buffer should activate them.

* Backends are just functions kept in a buffer-local variable. See
  flymake-diagnostic-functions.

* Some colorful fanciness in the mode-line. Still far from flycheck's
  (which is very good) but should be easy to get there.

* In a separate scrap-able commit I bind M-n and M-p in flymake-mode to
  navigate errors. I like it and the keys were unused, but it's probably
  against the rules.

* I started rewriting the manual, but won't go much further until the
  implementation is stabilized.

Regarding backward compatibility:

* I have provided obsolete aliases for most variables and functions,
  at least the ones I though were "public". Probably too many, but
  possibly missed one.

* Steve Purcell's flymake-easy.el adaptor
  (https://github.com/purcell/flymake-easy) seems to work, which is a
  nice surprise. It hooks onto the legacy backend via the obsolete
  aliases.

* Got rid of flymake-display-err-menu-for-current-line, since it wasn't
  doing anything useful anyway.

The code is in the scratch/flymake-refactor branch on Savannah. There
are some 40 proper Changelog-style commits with profuse comments
explaining the changes. You can follow that story or review the final
patch, but keep M-x vc-region-history handy.

Here are a couple of screenshots taken from clean emacs -Q runs after
just M-x flymake-mode.

[-- Attachment #2: flymake-1.png --]
[-- Type: image/png, Size: 42732 bytes --]

[-- Attachment #3: flymake-2.png --]
[-- Type: image/png, Size: 43895 bytes --]

Re: Flymake refactored

[Stefan Monnier]

Hi João,

This looks really good, thank you very much.  I think this kind of
functionality is very important and we should strive to polish it such
that it can be enabled by default globally, via a global-flymake-mode
(although I'm quite aware that we're not there yet and I don't think we
need to get there before your changes are merged).

I'm sorry I didn't give you feedback earlier, so here's my impressions
on a fairly quick look at the overall diff.

Note: Just because some are nitpicks about the specific working of
a specific chunk of code doesn't mean that I've read all the code
carefully (it's *really* not the case).  More to the point, I'm sure
I missed a lot of things.

> +line, respectively. When @code{flymake-mode} is active, they are
> +mapped to @kbd{M-n} and @kbd{M-p}, respectively, and by default.

Right, as you guessed I don't think this is acceptable for the default
(I use M-n/M-p bindings in my smerge-mode config and am pretty happy
with it, but I'm not even sure I'd like it for flymake-mode because it's
a mode that will be "always" enabled, contrary to smerge-mode which
I only enable while there are conflicts to resolve).

Maybe we could hook it into `next-error` so you can use C-x ` or `M-g n/p`
for it, but I'm not sure if it will work well (the problem with the
`next-error` thingy is that there can be many different "kinds" or
error sources, so flymake would end up competing with compile.el, grep,
etc...).

> +;; Copyright (C) 2017  João Távora

Of course, this should say FSF instead.

> +(defun flymake-elisp--checkdoc-1 ()
> +  "Do actual work for `flymake-elisp-checkdoc'."
> +  (let (collected)
> +    (cl-letf (((symbol-function 'checkdoc-create-error)
> +               (lambda (text start end &optional unfixable)
> +                 (push (list text start end unfixable) collected)
> +                 nil)))

We should change checkdoc.el directly to avoid such hackish surgery.

> +(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.

> +(defun flymake-elisp--batch-byte-compile (&optional file)
[...]
> +    (unwind-protect
> +        (cl-letf (((symbol-function 'byte-compile-log-warning)
> +                   (lambda (string &optional fill level)
> +                     (push (list string byte-compile-last-position fill level)
> +                           collected)
> +                     t)))

Similarly, we should change directly byte-compile-log-warning such that
this kind of surgery is not needed.

> +  (add-to-list (make-local-variable 'flymake-diagnostic-functions)
> +               'flymake-elisp-checkdoc t)
> +  (add-to-list (make-local-variable 'flymake-diagnostic-functions)
> +               'flymake-elisp-byte-compile t))

`flymake-diagnostic-functions` should be a(n abnormal) hook which we
manipulate with `add-hook`:

    (add-hook 'flymake-diagnostic-functions #'flymake-elisp-checkdoc nil t)
    (add-hook 'flymake-diagnostic-functions #'flymake-elisp-byte-compile nil t)

> +(add-hook 'emacs-lisp-mode-hook
> +          'flymake-elisp-setup-backends)

You should change elisp-mode.el directly instead.
Actually I'd argue that the contents of flymake-elisp.el should move
to elisp-mode.el.  This also means remove the (require 'flymake-ui)
and instead mark `flymake-make-diagnostic` as autoloaded.

> +(defcustom flymake-proc-allowed-file-name-masks
> +  '(("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'"
> +     flymake-proc-simple-make-init
> +     nil
> +     flymake-proc-real-file-name-considering-includes)
> +    ("\\.xml\\'" flymake-proc-xml-init)
> +    ("\\.html?\\'" flymake-proc-xml-init)
> +    ("\\.cs\\'" flymake-proc-simple-make-init)
> +    ("\\.p[ml]\\'" flymake-proc-perl-init)
> +    ("\\.php[345]?\\'" flymake-proc-php-init)
> +    ("\\.h\\'" flymake-proc-master-make-header-init flymake-proc-master-cleanup)
> +    ("\\.java\\'" flymake-proc-simple-make-java-init flymake-proc-simple-java-cleanup)
> +    ("[0-9]+\\.tex\\'" flymake-proc-master-tex-init flymake-proc-master-cleanup)
> +    ("\\.tex\\'" flymake-proc-simple-tex-init)
> +    ("\\.idl\\'" flymake-proc-simple-make-init)
>      ;; ("\\.cpp\\'" 1)
>      ;; ("\\.java\\'" 3)
>      ;; ("\\.h\\'" 2 ("\\.cpp\\'" "\\.c\\'")

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.

> +(add-to-list 'flymake-diagnostic-functions
> +             'flymake-proc-legacy-flymake)

Should also be

    (add-hook 'flymake-diagnostic-functions #'flymake-proc-legacy-flymake)

> +(progn
> +  (define-obsolete-variable-alias
[...]

There are too many obsolete aliases in there, IMO.
We should think a bit more about each one of them.

Many of them alias `flymake-foo` to `flymake-proc--bar`, which doesn't
make much sense: if `flymake-proc--bar` is used outside of flymake-proc
(via some old name), then it probably shouldn't have "--" in its name.

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).

Also some parts of flymake-proc seem to make sense for "all" backends
rather than only for flymake-proc.

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

> +  (define-obsolete-variable-alias 'flymake-xml-program
> +    'flymake-proc-xml-program "26.1"
> +    "Program to use for XML validation.")

In the long run, this should likely move to some xml major mode, so
`flymake-proc-xml-program` is probably not the "right" name for it.
So I think we may as well keep using `flymake-xml-program` for now,
until it's moved to some other package.

> +  (define-obsolete-variable-alias 'flymake-allowed-file-name-masks
> +    'flymake-proc-allowed-file-name-masks "26.1"

Since I argued above that flymake-proc-allowed-file-name-masks should
be obsolete, there's not much point renaming
flymake-allowed-file-name-masks to flymake-proc-allowed-file-name-masks.

> +  :group 'flymake

Inside flymake(-ui).el, those `:group 'flymake` are redundant.

> +(defun flymake-make-diagnostic (buffer
> +                                beg
> +                                end
> +                                type
> +                                text)
> +  "Mark BUFFER's region from BEG to END with a flymake diagnostic.

AFAIK this function does not really "mark".  It just creates
a diagnostic object which will/may cause the corresponding region to be
marked at some later time.

> +TYPE is a key to `flymake-diagnostic-types-alist' and TEXT is a
> +description of the problem detected in this region."
> +  (flymake--diag-make :buffer buffer :beg beg :end end :type type :text text))

You could get almost the same function (and completely skip the
definition of flymake--diag-make) with:

         (:constructor flymake-make-diagnostic (buffer beg end type text))

Maybe we should extend cl-defstruct so we can give it docstrings
for :constructors, since I suspect that the desire to provide a proper
docstring was the main motivation for not using a :constructor here.

> +  (cl-remove-if-not
> +   (lambda (ov)
> +     (and (overlay-get ov 'flymake-overlay)

`ov` is by definition an overlay, so using property name
`flymake-overlay` sounds redundant: just `flymake` would do just fine.
(actually it's a bit worse: the property name `flymake-overlay` makes it
sound like its value will be an overlay, whereas it's just t).

> +          (or (not filter)
> +              (cond ((functionp filter) (funcall filter ov))
> +                    ((symbolp filter) (overlay-get ov filter))))))
> +   (save-restriction
> +     (widen)
> +     (let ((ovs (if (and beg (null end))
> +                    (overlays-at beg t)
> +                  (overlays-in (or beg (point-min))
> +                               (or end (point-max))))))
> +       (if compare
> +           (cl-sort ovs compare :key (or key
> +                                         #'identity))
> +         ovs)))))

You probably should `cl-remove-if-not` before doing to `cl-sort`.
Both for performance reasons (fewer overlays to sort) and so that your
`compare` and `key` functions don't have to deal with
non-flymake overlays.

> +* A (possibly empty) list of objects created with
> +  `flymake-make-diagnostic', causing flymake to annotate the
> +  buffer with this information and consider the backend has
> +  having finished its check normally.

The docstring should say if it's OK for the backend to call REPORT-FN
several times with such diagnostics (so it can start a background
process and call the REPORT-FN every time it gets a chunk of reports
from the background process), or whether it should instead wait until it
has all the diagnostics before calling REPORT-FN.

> +* 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?

> +* The symbol `:panic', signalling that the backend has
> +  encountered an exceptional situation and should be disabled.
> +
> +In the latter cases, it is also possible to provide REPORT-FN
> +with a string as the keyword argument `:explanation'. The string
> +should give human-readable details of the situation.")

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))

Maybe just say that the third option is to pass to REPORT-FN a value of
the form `(:panic . EXPLANATION)` where EXPLANATION is a string.
[ Note: I don't like using &key for functions, so I recommend not to
  use it for API interfaces such as flymake-diagnostic-functions,
  although I don't object to using them internally in flymake if you so
  prefer.  ]

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.

> +(defvar flymake-diagnostic-types-alist
> +  `((:error
> +     . ((category . flymake-error)
> +        (mode-line-face . compilation-error)))
> +    (:warning
> +     . ((category . flymake-warning)
> +        (mode-line-face . compilation-warning)))
> +    (:note
> +     . ((category . flymake-note)
> +        (mode-line-face . compilation-info))))

This mapping from diag-type to properties seems redundant with the one
from category symbol to properties.  I'm no friend of the `category`
property, so I'd recommend you use

    (defvar flymake-diagnostic-types-alist
      `((:error (face . flymake-error)
                (bitmap . flymake-error-bitmap)
                (severity . ,(warning-numeric-level :error))
                (mode-line-face . compilation-error))
         ...))

but if you prefer using `category`, you can just use
(intern (format "flymake-category-%s" diag-type)) and then put all the
properties on those symbols.

> +        (dolist (backend flymake-diagnostic-functions)

In order to properly handle flymake-diagnostic-functions as a hook
(e.g. handle the t case), you probably want to use:

    (run-hook-wrapped 'flymake-diagnostic-functions
      (lambda (backend)
        (cond ((memq backend flymake--running-backends)
               (flymake-log 2 "Backend %s still running, not restarting"
                            backend))
              ((memq backend flymake--disabled-backends)
               (flymake-log 2 "Backend %s is disabled, not starting"
                            backend))
              (t
               (flymake--run-backend backend)))
        nil))

>  (defun flymake-mode-on ()
>    "Turn flymake mode on."
>    (flymake-mode 1)
> -  (flymake-log 1 "flymake mode turned ON for buffer %s" (buffer-name)))
> +  (flymake-log 1 "flymake mode turned ON"))

We should make this an obsolete alias of `flymake-mode`.

> -;;;###autoload
>  (defun flymake-mode-off ()
>    "Turn flymake mode off."
>    (flymake-mode 0)
> -  (flymake-log 1 "flymake mode turned OFF for buffer %s" (buffer-name)))
> +  (flymake-log 1 "flymake mode turned OFF"))

I'd make this obsolete (for those rare cases where it's needed, you can
just as well call (flymake-mode -1)).

> +        (reported (hash-table-keys flymake--diagnostics-table)))

AFAICT you only use `reported` as a boolean, so it would be more
efficient to use

           (reported (> (hash-table-count flymake--diagnostics-table) 0))

[ I noticed this because of `hash-table-keys`: almost every time it's
  used, it's an inefficient way to solve the problem, in my
  experience ;-)  ]

> +;;; Dummy autoloads ensure that this file gets autoloaded, not just
> +;;; flymake-ui.el where they actually live.
> +
> +;;;###autoload
> +(defun flymake-mode-on () "Turn flymake mode on." nil)
> +
> +;;;###autoload
> +(defun flymake-mode-off () "Turn flymake mode off." nil)
> +
> +;;;###autoload
> +(define-minor-mode flymake-mode nil)

Yuck!

> +(require 'flymake-ui)
> +(require 'flymake-proc)
> +(require 'flymake-elisp)

`flymake-elisp.el` should not be loaded as soon as you enable
flymake-mode, since we may never use Elisp in that session: the
dependency goes in the wrong direction.

`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.

I think the best solution is to make it so flymake.el doesn't (require
'flymake-proc).  It will require moving some stuff from flymake-proc.el
to flymake.el (e.g. all the backward compatibility APIs), but I think
the result may be even cleaner.


        Stefan

Re: Flymake refactored

[João Távora]

Hi Stefan,

> This looks really good, thank you very much.  I think this kind of
> functionality is very important and we should strive to polish it

Thanks, and thanks very much for such a speedy review. Before I start
replying to it, I just wanted to say that I'll have to switch context
again away from hacking very soon, so I wanted to get the most out of
these early review cycles

At the end of this mail, I've summarized a list of the changes you
suggested, from less to more controversial. If you see something trivial
that you can fix right away, or something less trivial but
uncontroversial, just push it to the branch.

> I'm sorry I didn't give you feedback earlier, so here's my impressions
> on a fairly quick look at the overall diff.

No problem. I've only picked it up the last week or so. It was sitting
pretty quiet in the scratch branch in a state that didn't really merit
reviewing. From my perspective, 6 hours between asking and getting a
decent review is pretty good.

> Right, as you guessed I don't think this is acceptable for the default
> (I use M-n/M-p bindings in my smerge-mode config and am pretty happy

Well, in your case I would like to use M-n/M-p for flymake and overriden
vwhen smerge-mode kicks in, which is a much more serious kind of
conflict. When smerge-mode turns itself off again (it doesn't lately,
but it used to), you get flymake again.

OTOH, perhaps it's a good thing they are empty. They should be reserved
for the user IMO.

> Maybe we could hook it into `next-error` so you can use C-x ` or `M-g n/p`
> for it, but I'm not sure if it will work well (the problem with the
> `next-error` thingy is that there can be many different "kinds" or
> error sources, so flymake would end up competing with compile.el, grep,
> etc...).

Agree. Let's not do anything :-). I didn't mention it, but you can
navigate through errors with the mouse wheel on the little red/yellow
indicators.

>> +;; Copyright (C) 2017  João Távora
>
> Of course, this should say FSF instead.

:-)

> 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. (...but didn't you use to be the advocate for add-function
everywhere, as in add-function versus hooks? Isn't this slightly
better?)

>> +(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. IDK, is it a good idea to admit
that (lazy) users place everything in the global hook? Inn which case it
should actually error, which is a synchronous form of panic that causes
the UI to disable it for the buffer.

>> +(defun flymake-elisp--batch-byte-compile (&optional file)
> [...]
>> +    (unwind-protect
>> +        (cl-letf (((symbol-function 'byte-compile-log-warning)
>> +                   (lambda (string &optional fill level)
>> +                     (push (list string byte-compile-last-position fill level)
>> +                           collected)
>> +                     t)))
>

>
>> +  (add-to-list (make-local-variable 'flymake-diagnostic-functions)
>> +               'flymake-elisp-checkdoc t)
>> +  (add-to-list (make-local-variable 'flymake-diagnostic-functions)
>> +               'flymake-elisp-byte-compile t))
>
> `flymake-diagnostic-functions` should be a(n abnormal) hook which we
> manipulate with `add-hook`:
>
>     (add-hook 'flymake-diagnostic-functions #'flymake-elisp-checkdoc nil t)
>     (add-hook 'flymake-diagnostic-functions
>     #'flymake-elisp-byte-compile nil t)

No problem. I actually wanted a hook, but no run-hook-* function had the the
proper semantics. And I didn't want to write the logic for the 't'
and everything. But now I notice run-hook-wrapped should do a nice job, right?

>> +(add-hook 'emacs-lisp-mode-hook
>> +          'flymake-elisp-setup-backends)
> You should change elisp-mode.el directly instead.

OK.

> Actually I'd argue that the contents of flymake-elisp.el should move
> to elisp-mode.el.

OK.

> 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. But isn't having flymake-make-diagnostic and the hook already
"knowing" about flymake)? Isn't (eval-when-compile (require
'flymake-ui)) better?  But maybe I don't read you correctly and I'm just
confused.

>> +(defcustom flymake-proc-allowed-file-name-masks
>> +  '(("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'"
>> +     flymake-proc-simple-make-init
>> +     nil
>> +     flymake-proc-real-file-name-considering-includes)
>> +    ("\\.xml\\'" flymake-proc-xml-init)
>> +    ("\\.html?\\'" flymake-proc-xml-init)
>> +    ("\\.cs\\'" flymake-proc-simple-make-init)
>> +    ("\\.p[ml]\\'" flymake-proc-perl-init)
>> +    ("\\.php[345]?\\'" flymake-proc-php-init)
>> +    ("\\.h\\'" flymake-proc-master-make-header-init flymake-proc-master-cleanup)
>> +    ("\\.java\\'" flymake-proc-simple-make-java-init flymake-proc-simple-java-cleanup)
>> +    ("[0-9]+\\.tex\\'" flymake-proc-master-tex-init flymake-proc-master-cleanup)
>> +    ("\\.tex\\'" flymake-proc-simple-tex-init)
>> +    ("\\.idl\\'" flymake-proc-simple-make-init)
>>      ;; ("\\.cpp\\'" 1)
>>      ;; ("\\.java\\'" 3)
>>      ;; ("\\.h\\'" 2 ("\\.cpp\\'" "\\.c\\'")
>
> 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.

>> +(progn
>> +  (define-obsolete-variable-alias
> [...]
>
> There are too many obsolete aliases in there, IMO.
> We should think a bit more about each one of them.

Oh no :-)
>
> Many of them alias `flymake-foo` to `flymake-proc--bar`, which doesn't
> make much sense: if `flymake-proc--bar` is used outside of flymake-proc
> (via some old name), then it probably shouldn't have "--" in its name.

Makes a lot of sense, I removed them.

When I started I too obsessed with compatibility so you might have
guessed I started by making one alias for every symbol indiscriminately,
marked it internal for good measure. Then I hand picked some that looked
like they could be used outside.

Now I consider flymake-easy.el working minimally to be sufficient backward
compatibility.

> 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.

> 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.
And fixed: the "master" mechanism (good old "master" naming eh), for
example, is broken. It cannot understand that C++ .tpp files are
included from .hpp included from .cpp.

Don't get me wrong, legacy flymake has been immensely useful for me, but
again I think it should stay put until we have a better alternative. I
think we could handle a bit of duplication here.

> 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.

>> +  (define-obsolete-variable-alias 'flymake-xml-program
>> +    'flymake-proc-xml-program "26.1"
>> +    "Program to use for XML validation.")
>
> In the long run, this should likely move to some xml major mode, so
> `flymake-proc-xml-program` is probably not the "right" name for it.
> So I think we may as well keep using `flymake-xml-program` for now,
> until it's moved to some other package.

No, let's write a good backend for xml-mode/sgml-modes instead. There
are probably super cool xml linters out there, some perhaps in elisp
(nxml-mode of James Clark fame for one)

>> +  :group 'flymake
>
> Inside flymake(-ui).el, those `:group 'flymake` are redundant.

A nitpick indeed :-). Fixed.

>> +(defun flymake-make-diagnostic (buffer
>> +                                beg
>> +                                end
>> +                                type
>> +                                text)
>> +  "Mark BUFFER's region from BEG to END with a flymake diagnostic.
>
> AFAIK this function does not really "mark".  It just creates

It used to. Fixed the docstring.

>
>> +TYPE is a key to `flymake-diagnostic-types-alist' and TEXT is a
>> +description of the problem detected in this region."
>> +  (flymake--diag-make :buffer buffer :beg beg :end end :type type :text text))
>
> You could get almost the same function (and completely skip the
> definition of flymake--diag-make) with:
>
>          (:constructor flymake-make-diagnostic (buffer beg end type
>          text))
> Maybe we should extend cl-defstruct so we can give it docstrings
> for :constructors, since I suspect that the desire to provide a proper
> docstring was the main motivation for not using a :constructor here.

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. 

>> +  (cl-remove-if-not
>> +   (lambda (ov)
>> +     (and (overlay-get ov 'flymake-overlay)
>
> `ov` is by definition an overlay, so using property name
> `flymake-overlay` sounds redundant: just `flymake` would do just fine.
> (actually it's a bit worse: the property name `flymake-overlay` makes it
> sound like its value will be an overlay, whereas it's just t).

Not just "a bit worse", Stefan: a truly horrible crime. Fixed.

> You probably should `cl-remove-if-not` before doing to `cl-sort`.
> Both for performance reasons (fewer overlays to sort) and so that your
> `compare` and `key` functions don't have to deal with
> non-flymake overlays.

Well spotted. Fixed.

> The docstring should say if it's OK for the backend to call REPORT-FN
> several times with such diagnostics (so it can start a background
> process and call the REPORT-FN every time it gets a chunk of reports
> from the background process), or whether it should instead wait until it
> has all the diagnostics before calling REPORT-FN.

Indeed. I think it should be OK to do so, but currently it
isn't. Something to think about. Since each REPORT-FN is a different
lambda we could use that fact to track if a calling it "too much".

>> +* 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 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") 

> Maybe just say that the third option is to pass to REPORT-FN a value of
> the form `(:panic . EXPLANATION)` where EXPLANATION is a string.
> [ Note: I don't like using &key for functions, so I recommend not to
>   use it for API interfaces such as flymake-diagnostic-functions,
>   although I don't object to using them internally in flymake if you so
>   prefer.  ]

The first argument to REPORT-FN can be a list or a non-nil symbol. In
the latter case keywords are accepted. Actually :explanation is always
accepted, as is :force. I just forgot to describe it. Probably other
keywords will come in handy. Keywords are good, they're free
destructuring, better than pcase'ing funky cons (much as I respect pcase
:-).

> 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. 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.

>> +(defvar flymake-diagnostic-types-alist
>> +  `((:error
>> +     . ((category . flymake-error)
>> +        (mode-line-face . compilation-error)))
>> +    (:warning
>> +     . ((category . flymake-warning)
>> +        (mode-line-face . compilation-warning)))
>> +    (:note
>> +     . ((category . flymake-note)
>> +        (mode-line-face . compilation-info))))
>
> This mapping from diag-type to properties seems redundant with the one
> from category symbol to properties.  I'm no friend of the `category`
> property, so I'd recommend you use
>
>     (defvar flymake-diagnostic-types-alist
>       `((:error (face . flymake-error)
>                 (bitmap . flymake-error-bitmap)
>                 (severity . ,(warning-numeric-level :error))
>                 (mode-line-face . compilation-error))
>          ...))

How would I mixin existing stuff for a supposed :my-error that is
just like :error, but overrides some properties.

> but if you prefer using `category`, you can just use
> (intern (format "flymake-category-%s" diag-type)) and then put all the
> properties on those symbols.

OK. Will do.

>> +        (dolist (backend flymake-diagnostic-functions)
>
> In order to properly handle flymake-diagnostic-functions as a hook
> (e.g. handle the t case), you probably want to use:
>
>     (run-hook-wrapped 'flymake-diagnostic-functions
>       (lambda (backend)
>         (cond ((memq backend flymake--running-backends)
>                (flymake-log 2 "Backend %s still running, not restarting"
>                             backend))
>               ((memq backend flymake--disabled-backends)
>                (flymake-log 2 "Backend %s is disabled, not starting"
>                             backend))
>               (t
>                (flymake--run-backend backend)))
>         nil))

That's it (I had seen run-hook-wrapped in the meantime). Thanks!

>>  (defun flymake-mode-on ()
>>    "Turn flymake mode on."
>>    (flymake-mode 1)
>> -  (flymake-log 1 "flymake mode turned ON for buffer %s" (buffer-name)))
>> +  (flymake-log 1 "flymake mode turned ON"))
>
> We should make this an obsolete alias of `flymake-mode`.

OK.

>> -;;;###autoload
>>  (defun flymake-mode-off ()
>>    "Turn flymake mode off."
>>    (flymake-mode 0)
>> -  (flymake-log 1 "flymake mode turned OFF for buffer %s" (buffer-name)))
>> +  (flymake-log 1 "flymake mode turned OFF"))
>
> I'd make this obsolete (for those rare cases where it's needed, you can
> just as well call (flymake-mode -1)).

OK.

>> +        (reported (hash-table-keys flymake--diagnostics-table)))
>
> AFAICT you only use `reported` as a boolean, so it would be more
> efficient to use
>
>            (reported (> (hash-table-count flymake--diagnostics-table) 0))
>
> [ I noticed this because of `hash-table-keys`: almost every time it's
>   used, it's an inefficient way to solve the problem, in my
>   experience ;-)  ]

Actually, I was thinking of making cl-set-difference assertions between
that and other sets in the future. hash-table-keys seems a nice way.

>> +;;; Dummy autoloads ensure that this file gets autoloaded, not just
>> +;;; flymake-ui.el where they actually live.
>>
>> +;;;###autoload
>> +(define-minor-mode flymake-mode nil)
>
> Yuck!

Yes, kinda horrible. I was in a hurry.

>> +(require 'flymake-ui)
>> +(require 'flymake-proc)
>> +(require 'flymake-elisp)
>
> `flymake-elisp.el` should not be loaded as soon as you enable
> flymake-mode, since we may never use Elisp in that session: the
> dependency goes in the wrong direction.

OK.

> `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?

> I think the best solution is to make it so flymake.el doesn't (require
> 'flymake-proc).  It will require moving some stuff from flymake-proc.el
> to flymake.el (e.g. all the backward compatibility APIs), but I think
> the result may be even cleaner.

I think that's my idea from the previous paragraph too. OK.

To summarize:

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

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.

Will fix, but harder:

* proper checkdoc.el interface
* proper byte-compile.el interface
* Move flymake-elisp.el contents to elisp-mode.el
* Allow REPORT-FN to be called multiple times from backends
* Passing recent changes to backends
* Rename flymake-ui.el to flymake.el and make some autoloads.

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.

Don't like:
* removing category in flymake-diagnostic-types-alist
* mark flymake-proc-err-line-patterns obsolete and add to
  some other variable from across emacs progmodes.
* remove the -proc prefix from defs in the proc backend.
* reconsider parts from flymake-proc into an util package.

No effect:

* Copyright FSF in the header

Thanks,
João

Re: Flymake refactored

[Stefan Monnier]

>> 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

Re: Flymake refactored

[João Távora]

[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 <monnier@iro.umontreal.ca> 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ão

Re: Flymake refactored

[Stefan Monnier]

> [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]

That's OK.  We want to allow this in scratch branches.

> 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.

I saw your recent introduction of proper hooks for checkdoc
and bytecomp.  I intend to take a closer look at them, but haven't had
time for it yet.

>> 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)

"Hard to do generically + not particularly useful" qualifies as
"with regret" I think ;-)

>> 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?

I don't actually know.  But since nxml's checker currently doesn't go
though flymake.el its design was special tailored to take advantage of
the info available in Emacs (e.g. knowledge of what was changed, to
avoid repeating previous checks, knowledge of what's displayed, so as
to do all the checks "instantly" yet lazily, ...).  I can't remember
enough of the details to be sure, but my gut feeling tells me that it'll
be hard to preserve the desirable properties while interfacing
through flymake (since it's targeted at a very different use case).

> Actually, I just (require 'flymake-proc) *after* (provide 'flymake) in
> flymake.el. It looks pretty clean to me, does you see any drawbacks?

Yes, I saw that.  It's indeed simple, and should work fine especially
for a file which we plan on marking obsolete.

>> 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?

Don't know.  flymake-diagnostic-functions?


        Stefan

Re: Flymake refactored

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> That's OK.  We want to allow this in scratch branches.

Yeah, but boy does it spam emacs-diffs. Can we enable push -f in just
the scratch branches. I don't know if its possible but Google says it's
a pretty commonly requested feature.

> "Hard to do generically + not particularly useful" qualifies as
> "with regret" I think ;-)

Au contraire, with delight, it's not everyday that a hard thing to do is
useless :-)

> enough of the details to be sure, but my gut feeling tells me that it'll
> be hard to preserve the desirable properties while interfacing
> through flymake (since it's targeted at a very different use case).

I don't know (I also haven't looked at the code) but none of those
things sound like showstoppers.  For a start, I'd be happy just
preserving the fact that it isn't based on regexps.  The only thing a
backend has to do is tell flymake where some problems exist. Even a naive
solution like running nmxl in a subprocess emacs and prin1'int a list of
collected errors, like we do for elisp-flymake-byte-compile now,
shouldn't be horrible.

>> Didn't do this too.  If we mark it obsolete, what's the "use instead"
>> message?
> Don't know.  flymake-diagnostic-functions?

Yeah, but right now that's saying "this bit is obsolete, go write a
replacement and then use that instead. good luck "

Regarding the merge to emacs-26, do you see anything else we need to
iron out before it?

João

Re: Flymake refactored

[Stefan Monnier]

>> enough of the details to be sure, but my gut feeling tells me that it'll
>> be hard to preserve the desirable properties while interfacing
>> through flymake (since it's targeted at a very different use case).

> I don't know (I also haven't looked at the code) but none of those
> things sound like showstoppers.  For a start, I'd be happy just
> preserving the fact that it isn't based on regexps.  The only thing a
> backend has to do is tell flymake where some problems exist. Even a naive
> solution like running nmxl in a subprocess emacs and prin1'int a list of
> collected errors, like we do for elisp-flymake-byte-compile now,
> shouldn't be horrible.

Oh, I don't forsee any major difficulty in writing an nxml backend for
flymake, *IF* we limit ourselves to the goal of making it work.  But if
we want it to work about as well as nxml-mode itself, it'll be harder.

>>> Didn't do this too.  If we mark it obsolete, what's the "use instead"
>>> message?
>> Don't know.  flymake-diagnostic-functions?
> Yeah, but right now that's saying "this bit is obsolete, go write a
> replacement and then use that instead. good luck "

I thought you were the one saying that flymake-proc is all legacy and
will be replaced.  I don't think anyone has claimed that flymake-proc is
*currently* obsolete, just that the plan is to retire it at some point
(this point being presumably after a replacement is written).

> Regarding the merge to emacs-26, do you see anything else we need to
> iron out before it?

Maybe just this issue of letting backends tell flymake.el whey they're
done (and letting flymake tell backends to abort the currently running
check)?


        Stefan

Re: Flymake refactored

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Oh, I don't forsee any major difficulty in writing an nxml backend for
> flymake, *IF* we limit ourselves to the goal of making it work.  But if
> we want it to work about as well as nxml-mode itself, it'll be harder.

Maybe you're right for the bit where (from what I gather from your
comments) nxml-mode concentrates on the visible part of the buffer. Not
unrelated, that's where nxml-mode and Flymake differ in their
promise. Flymake's is to check the whole buffer, letting you know in the
mode-line how many errors you have, and quickly letting you navigate
between them. I think that, by design, that part could never be "as well
as nxml-mode". On the other hand, perhaps it would be useful to ask
backends to concentrate on this region first, then elsewhere.

>>>> Didn't do this too.  If we mark it obsolete, what's the "use instead"
>>>> message?
>>> Don't know.  flymake-diagnostic-functions?
>> Yeah, but right now that's saying "this bit is obsolete, go write a
>> replacement and then use that instead. good luck "
>
> I thought you were the one saying that flymake-proc is all legacy and
> will be replaced.  I don't think anyone has claimed that flymake-proc is
> *currently* obsolete, just that the plan is to retire it at some point
> (this point being presumably after a replacement is written).

OK then. I guess I misunderstand the graveness of a make-obsolete.

>> Regarding the merge to emacs-26, do you see anything else we need to
>> iron out before it?
>
> Maybe just this issue of letting backends tell flymake.el whey they're
> done (and letting flymake tell backends to abort the currently running
> check)?

Done, and rebuilt a prettier history in the branch
scratch/flymake-refactor-cleaner-for-emacs-26.

Will still fix some more longstanding bugs and then merge to
emacs-26. Bugs and finishing touches can still be fixed there.

João

Re: Flymake refactored

[Stefan Monnier]

> Will still fix some more longstanding bugs and then merge to
> emacs-26. Bugs and finishing touches can still be fixed there.

Sounds good to me,


        Stefan

Re: Flymake refactored

[Dmitry Gutov]

On 9/28/17 9:52 PM, Stefan Monnier wrote:

> Maybe we could hook it into `next-error` so you can use C-x ` or `M-g n/p`
> for it, but I'm not sure if it will work well (the problem with the
> `next-error` thingy is that there can be many different "kinds" or
> error sources, so flymake would end up competing with compile.el, grep,
> etc...).

I think it would be very silly to use next-error for grep, but not for 
flymake. Flycheck uses it, so flymake can make do somehow, too.

It should also encourage us to sort out that mess.

Re: Flymake refactored

[Ted Zlatanov]

On Fri, 29 Sep 2017 14:51:59 +0200 Dmitry Gutov <dgutov@yandex.ru> wrote: 

DG> On 9/28/17 9:52 PM, Stefan Monnier wrote:
>> Maybe we could hook it into `next-error` so you can use C-x ` or `M-g n/p`
>> for it, but I'm not sure if it will work well (the problem with the
>> `next-error` thingy is that there can be many different "kinds" or
>> error sources, so flymake would end up competing with compile.el, grep,
>> etc...).

DG> I think it would be very silly to use next-error for grep, but not for flymake.
DG> Flycheck uses it, so flymake can make do somehow, too.

DG> It should also encourage us to sort out that mess.

Right now the logic is based on buffer visibility, I think. next-error
supposed to be DWIM so the complexity is on the software side (and
messy) to keep the user experience simple. For me, it Just Works
currently (I use Occur and Flycheck a lot, compile and grep/git grep
less) so I've been pretty happy with it. I'm sure it can be improved :)

Ted

Re: Flymake refactored

[Dmitry Gutov]

On 9/29/17 4:55 PM, Ted Zlatanov wrote:

> Right now the logic is based on buffer visibility, I think. next-error
> supposed to be DWIM so the complexity is on the software side (and
> messy) to keep the user experience simple. For me, it Just Works
> currently (I use Occur and Flycheck a lot, compile and grep/git grep
> less) so I've been pretty happy with it. I'm sure it can be improved :)

See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489 for a recent 
discussion.

Re: Flymake refactored

[Ted Zlatanov]

On Fri, 29 Sep 2017 17:03:52 +0200 Dmitry Gutov <dgutov@yandex.ru> wrote: 

DG> On 9/29/17 4:55 PM, Ted Zlatanov wrote:
>> Right now the logic is based on buffer visibility, I think. next-error
>> supposed to be DWIM so the complexity is on the software side (and
>> messy) to keep the user experience simple. For me, it Just Works
>> currently (I use Occur and Flycheck a lot, compile and grep/git grep
>> less) so I've been pretty happy with it. I'm sure it can be improved :)

DG> See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489 for a recent discussion.

I was specifically talking about how Flymake should integrate with
next-error. Sorry I didn't make that clearer.

I'm not sure if you want to follow up on that bug or here in a new
thread, but if you want to make a branch with the changes you think
would improve the next-error DWIM experience, I'll be glad to take a
look and test it with my workflows. That would probably be the most
effective way.

Ted

Re: Flymake refactored

[Dmitry Gutov]

On 9/29/17 6:26 PM, Ted Zlatanov wrote:

> I was specifically talking about how Flymake should integrate with
> next-error. Sorry I didn't make that clearer.

It's still not clear to me, then. Should Flymake integration be "messy"?

> I'm not sure if you want to follow up on that bug or here in a new
> thread, but if you want to make a branch with the changes you think
> would improve the next-error DWIM experience, I'll be glad to take a
> look and test it with my workflows. That would probably be the most
> effective way.

Hopefully, someday. Right now, I see how it shouldn't work, but I don't 
exactly see how it should.

Re: Flymake refactored

[Ted Zlatanov]

On Fri, 29 Sep 2017 19:35:38 +0200 Dmitry Gutov <dgutov@yandex.ru> wrote: 

DG> On 9/29/17 6:26 PM, Ted Zlatanov wrote:
>> I was specifically talking about how Flymake should integrate with
>> next-error. Sorry I didn't make that clearer.

DG> It's still not clear to me, then. Should Flymake integration be "messy"?

I'd go for "unsurprising" :)

>> I'm not sure if you want to follow up on that bug or here in a new
>> thread, but if you want to make a branch with the changes you think
>> would improve the next-error DWIM experience, I'll be glad to take a
>> look and test it with my workflows. That would probably be the most
>> effective way.

DG> Hopefully, someday. Right now, I see how it shouldn't work, but I don't exactly
DG> see how it should.

It might be productive to gather use cases and workflows. Those were
missing in the bug discussion, and will probably make it clearer.

For instance, my typical Flycheck interaction while writing CFEngine
code is to keep 1-2 cfengine-mode buffers open (next-error just
navigates between syntax errors in the visible buffer) and to sometimes
pop up an Occur buffer (in which case I want to navigate ocurrences for
as long as the Occur buffer is visible). I generally don't compile,
grep, or git grep in that workflow.

HTH
Ted

Re: Flymake refactored

[Dmitry Gutov]

On 9/29/17 7:56 PM, Ted Zlatanov wrote:

> DG> It's still not clear to me, then. Should Flymake integration be "messy"?
> 
> I'd go for "unsurprising" :)

Then the concerns about M-x next-error should still apply.

> It might be productive to gather use cases and workflows. Those were
> missing in the bug discussion, and will probably make it clearer.
> 
> For instance, my typical Flycheck interaction while writing CFEngine
> code is to keep 1-2 cfengine-mode buffers open (next-error just
> navigates between syntax errors in the visible buffer) and to sometimes
> pop up an Occur buffer (in which case I want to navigate ocurrences for
> as long as the Occur buffer is visible). I generally don't compile,
> grep, or git grep in that workflow.

I wouldn't say I have a specific workflow, but I do have a habit of 
killing or quitting windows or otherwise hiding buffers which I'm not 
looking at directly.

But there are some scenarios mentioned in the bug discussion, e.g. in 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489#74, earlier in 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489#44, and earlier 
again in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489#32.

Please send any further replies to this to the bug address.

Re: Flymake refactored

[Marcin Borkowski]

Hi,

I didn't (yet) look at it, but thanks for working on this.  I use
Flycheck currently (maily for JavaScript/ESlint), but I'm wondering
whether to switch to flymake at some point in time.  Just being curious:
does Flycheck support running more linters on the same file at the same
time?  (From a cursory glance at the docs, I'm not sure.)  Also, how
would one switch to Flymake for JS?

(Note: I understand that your time is limited, João, so if answer to any
of these questions requires more than 30 seconds, just tell me to rtfm,
which I'll be happy to do once I'm at home.)

Best,

--
Marcin Borkowski

Re: Flymake refactored

[João Távora]

Marcin Borkowski <mbork@mbork.pl> writes:

> I didn't (yet) look at it, but thanks for working on this.  I use
> Flycheck currently (maily for JavaScript/ESlint), but I'm wondering
> whether to switch to flymake at some point in time.  Just being curious:
> does Flycheck support running more linters on the same file at the same

I don't know (did you mean Flycheck or Flymake?). If you meant Flymake,
then the answer is yes.

> time?  (From a cursory glance at the docs, I'm not sure.)  Also, how
> would one switch to Flymake for JS?

One possibility would be: wait that it is (hopefully) present in the
next official emacs release, Emacs 26.1 and then wait that someone
writes a decent JS backend (what you call "linter" and Flycheck calls
"checkers") for it.

Another possibility would be: check out the most recent code and write
the backend yourself :-)

> (Note: I understand that your time is limited, João, so if answer to any
> of these questions requires more than 30 seconds, just tell me to rtfm,
> which I'll be happy to do once I'm at home.)

:-)

It took about 60 seconds but I'm not going to charge for the extra 30.

Also there is yet no m to rtf. Working on it.

João

Re: Flymake refactored

[Marcin Borkowski]

On 2017-10-01, at 01:43, João Távora <joaotavora@gmail.com> wrote:

> Marcin Borkowski <mbork@mbork.pl> writes:
>
>> I didn't (yet) look at it, but thanks for working on this.  I use
>> Flycheck currently (maily for JavaScript/ESlint), but I'm wondering
>> whether to switch to flymake at some point in time.  Just being curious:
>> does Flycheck support running more linters on the same file at the same
>
> I don't know (did you mean Flycheck or Flymake?). If you meant Flymake,
> then the answer is yes.

I know that.  I want to know whether Flycheck can do that, too.  If not,
then we have a strong advantage of Flymake.

>> time?  (From a cursory glance at the docs, I'm not sure.)  Also, how
>> would one switch to Flymake for JS?
>
> One possibility would be: wait that it is (hopefully) present in the
> next official emacs release, Emacs 26.1 and then wait that someone
> writes a decent JS backend (what you call "linter" and Flycheck calls
> "checkers") for it.

By "linter" I mean the outside program doing the checking (like
ESlint).  By "checker", Flymake/Flycheck mean the Elisp interface
between the linter and fly.*.

> Another possibility would be: check out the most recent code and write
> the backend yourself :-)

That's a cool idea.  I am quite tempted to do that.  As usual, time is
the main problem.

>> (Note: I understand that your time is limited, João, so if answer to any
>> of these questions requires more than 30 seconds, just tell me to rtfm,
>> which I'll be happy to do once I'm at home.)
>
> :-)
>
> It took about 60 seconds but I'm not going to charge for the extra 30.

Oh, thanks!

> Also there is yet no m to rtf. Working on it.

I'd be happy to proofread it when the time comes.

Best,

--
Marcin Borkowski

Re: Flymake refactored

[Mark Oteiza]

Marcin Borkowski <mbork@mbork.pl> writes:

> On 2017-10-01, at 01:43, João Távora <joaotavora@gmail.com> wrote:
>
>> Marcin Borkowski <mbork@mbork.pl> writes:
>>
>>> I didn't (yet) look at it, but thanks for working on this.  I use
>>> Flycheck currently (maily for JavaScript/ESlint), but I'm wondering
>>> whether to switch to flymake at some point in time.  Just being curious:
>>> does Flycheck support running more linters on the same file at the same
>>
>> I don't know (did you mean Flycheck or Flymake?). If you meant Flymake,
>> then the answer is yes.
>
> I know that.  I want to know whether Flycheck can do that, too.  If not,
> then we have a strong advantage of Flymake.

It can, but only does so if a particular checker specifies it with the
:next-checkers keyword.  For instance, the 'emacs-lisp' checker
indicates that the 'emacs-lisp-checkdoc' checker be run next.

Re: Flymake refactored

[Simen Heggestøyl]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

On Thu, Sep 28, 2017 at 4:27 PM, João Távora <joaotavora@gmail.com> 
wrote:
> * Steve Purcell's flymake-easy.el adaptor
>   (https://github.com/purcell/flymake-easy) seems to work, which is a
>   nice surprise. It hooks onto the legacy backend via the obsolete
>   aliases.

(I haven't really used Flymake or flymake-easy, so please excuse me if
the following question doesn't make any sense.)

Would it make sense to merge flymake-easy into Flymake while you're at
it, if it makes Flymake easier to use out of the box (as the package
name suggests)?

-- Simen

[-- Attachment #2: Type: text/html, Size: 879 bytes --]

Re: Flymake refactored

[João Távora]

Simen Heggestøyl <simenheg@gmail.com> writes:

> (I haven't really used Flymake or flymake-easy, so please excuse me if
> the following question doesn't make any sense.)
>
> Would it make sense to merge flymake-easy into Flymake while you're at
> it, if it makes Flymake easier to use out of the box (as the package
> name suggests)?

You question makes sense, but I don't favor doing that for two reasons:

* It would detract from the new API I'm trying to promote;
* It keeps Emacs tied to the legacy backend;

Of course if flymake-easy decides to hook onto the new API and still
provide its declarative interface, I have nothing against that.

But I hope the new API is simple enough to render even that
superfluous.

For example, here's a decent Ruby checker under 40 lines that does the
same as MELPA's flymake-ruby.el (which is based on flymake-easy), but
using the new API and without creating any temporary files.

João

(defvar-local ruby--flymake-proc nil)

(defun ruby-flymake (report-fn &rest _args)
  (unless (executable-find
           (car ruby-flymake-command)) (error "Cannot find a suitable ruby"))
  ;; Kill any obsolete processes, then create a new
  (when (process-live-p ruby--flymake-proc) (kill-process ruby--flymake-proc))
  (let ((source (current-buffer)))
    (save-restriction
      (widen)
      (setq ruby--flymake-proc
            (make-process
	     :name "ruby-flymake" :noquery t :connection-type 'pipe
	     :buffer (generate-new-buffer " *ruby-flymake*")
	     :command '("ruby" "-w" "-c")
	     :sentinel
	     (lambda (proc _event)
	       (unwind-protect
	           (with-current-buffer (process-buffer proc)
		     (goto-char (point-min))
		     (cl-loop
		      while (search-forward-regexp
		             "^\\(?:.*.rb\\|-\\):\\([0-9]+\\): \\(.*\\)$" nil t)
                      for msg = (match-string 2)
		      for (beg . end) = (flymake-diag-region
                                         source
                                         (string-to-number (match-string 1)))
		      for type = (if (string-match "^warning" msg) :warning :error)
		      collect (flymake-make-diagnostic source beg end type msg)
		      into diags
                      finally (funcall report-fn diags)))
	         (kill-buffer (process-buffer proc))))))
      (process-send-region ruby--flymake-proc (point-min) (point-max))
      (process-send-eof ruby--flymake-proc))))



>
> -- Simen

Re: Flymake refactored

[Mark Oteiza]

joaotavora@gmail.com (João Távora) writes:

> For example, here's a decent Ruby checker under 40 lines that does the
> same as MELPA's flymake-ruby.el (which is based on flymake-easy), but
> using the new API and without creating any temporary files.

Modeled after your example and bits from flycheck[0] and syntastic[1], I
have attached humble beginnings to a clang checker (errors only).

Something is probably very broken, as using it to check Emacs C at this
point triggers the following:

  funcall-interactively: binding stack not balanced (serious byte compiler bug)

I also seem to have problems using the column number (match-string 2)
and so it is left unused.

[0]: http://www.flycheck.org/en/latest/
[1]: https://github.com/vim-syntastic/syntastic

(defvar clang-program "clang")

(defvar-local clang--flymake-proc nil)

(defun clang-flymake (report-fn &rest _args)
  (unless (executable-find clang-program)
    (error "Cannot find a suitable clang"))
  (when (process-live-p clang--flymake-proc)
    (kill-process clang--flymake-proc))
  (let ((source (current-buffer)))
    (save-restriction
      (widen)
      (setq clang--flymake-proc
            (make-process
             :name "clang-flymake"
             :buffer (generate-new-buffer "*clang-flymake*")
             :command `(,clang-program
                        "-fsyntax-only" "-Weverything"
                        "-fno-color-diagnostics"
                        "-fno-caret-diagnostics"
                        "-fno-diagnostics-show-option"
                        "-x" "c" "-")
             :noquery t :connection-type 'pipe
             :sentinel
             (lambda (p _ev)
               (unwind-protect
                   (with-current-buffer (process-buffer p)
                     (goto-char (point-min))
                     (cl-loop
                      while (search-forward-regexp
                             (rx bol (or "<stdin>" (eval (or (buffer-file-name) "")))
                                 ":" (group-n 1 (+ digit)) ":" (group-n 2 (+ digit))
                                 ": " (or "fatal error" "error")
                                 ": " (group-n 3 (? (* anything))) eol)
                             nil t)
                      for msg = (match-string 3)
                      for (beg . end) = (flymake-diag-region
                                         (string-to-number (match-string 1)))
                      for type = :error
                      collect (flymake-make-diagnostic source beg end type msg)
                      into diags
                      finally (funcall report-fn diags)))
                 ;; (message "%S"
                 ;;          (with-current-buffer (process-buffer p)
                 ;;            (buffer-string)))
                 (kill-buffer (process-buffer p)))
               )))
      (process-send-region clang--flymake-proc (point-min) (point-max))
      (process-send-eof clang--flymake-proc))))

(defun clang-flymake-register ()
  (add-hook 'flymake-diagnostic-functions 'clang-flymake nil t))

(add-hook 'c-mode-hook 'clang-flymake-register)

Re: Flymake refactored

[João Távora]

Mark Oteiza <mvoteiza@udel.edu> writes:

> Modeled after your example and bits from flycheck[0] and syntastic[1], I
> have attached humble beginnings to a clang checker (errors only).

Looks great.

> Something is probably very broken, as using it to check Emacs C at this
> point triggers the following:
>
>   funcall-interactively: binding stack not balanced (serious byte compiler bug)

Sounds serious indeed. I've seen this too, once or twice, no idea how to
debug it.

> I also seem to have problems using the column number (match-string 2)
> and so it is left unused.

The problem is that you are calling flymake-diag-region in the wrong
buffer. It has to be the source buffer, so in your case you need a
(with-current-buffer source ...) around it.

But that inconvenience has been fixed in the very latest code emacs-26
branch. In that version, you just pass 'source' (the c/c++ buffer) to
flymake-diag-region.

>              :sentinel
>              (lambda (p _ev)

One of the things you must do here is check if your process is obsolete,
i.e. if Flymake decided to launch another one in the meantime. A good
way to do this is to check if 'p' is 'eq' to the buffer-local value of
clang-flymake--procress.

>                       for (beg . end) = (flymake-diag-region
>                                          (string-to-number
>                                          (match-string 1)))

This is the bit where you would pass 'source' to flymake-diag-region

I've built a backend very similar to yours but base on gcc (clang is
500MB and no time for that right now). Have a look, below my sig.

I've also noticed, there's a lot of repetition building up in these
examples. Later it's probably useful to invent an abstraction that hides
it away.

João

;;; test-gcc-backend.el --- naive gcc Flymake backend -*- lexical-binding: t; -*-

(defvar-local gcc--flymake-proc nil)

(defvar gcc-program "gcc")

(defun gcc-flymake (report-fn &rest _args)
  (unless (executable-find gcc-program)
    (error "Cannot find a suitable gcc"))
  (when (process-live-p gcc--flymake-proc)
    (kill-process gcc--flymake-proc))
  (let ((source (current-buffer)))
    (save-restriction
      (widen)
      (setq gcc--flymake-proc
            (make-process
             :name "gcc-flymake"
             :buffer (generate-new-buffer "*gcc-flymake*")
             :command `(,gcc-program
                        "-fsyntax-only" "-Wextra" "-Wall"
                        "-fno-diagnostics-show-option"
                        "-x" "c" "-")
             :noquery t :connection-type 'pipe
             :sentinel
             (lambda (p _ev)
               (unwind-protect
                   (when (eq p gcc--flymake-proc)
                     (with-current-buffer (process-buffer p)
                       (goto-char (point-min))
                       (cl-loop
                        while (search-forward-regexp
		               "^<stdin>:\\([0-9]+\\):\\([0-9]+\\): \\(.*\\): \\(.*\\)$"
		               nil t)
                        for msg = (match-string 4)
                        for (beg . end) = (flymake-diag-region source
                                                               (string-to-number (match-string 1))
                                                               (string-to-number (match-string 2)))
                        for type = (assoc-default (match-string 3)
                                                  '(("error" . :error)
                                                    ("note" . :note)
                                                    ("warning" . :warning))
                                                  #'string-match)
                        collect (flymake-make-diagnostic source beg end type msg)
                        into diags
                        finally (funcall report-fn diags))))
                 (display-buffer (process-buffer p)) ; use this instead of the next one for debug
                 ;; (kill-buffer (process-buffer p))
                 )
               )))
      (process-send-region gcc--flymake-proc (point-min) (point-max))
      (process-send-eof gcc--flymake-proc))))

Re: Flymake refactored

[Stefan Monnier]

>> :sentinel
>> (lambda (p _ev)
> One of the things you must do here is check if your process is obsolete,
> i.e. if Flymake decided to launch another one in the meantime. A good
> way to do this is to check if 'p' is 'eq' to the buffer-local value of
> clang-flymake--procress.

While I agree that it should check whether `p` is obsolete (just in case
something went wrong elsewhere), `p` should have been killed when the
other process was launched, so this sentinel should only be called with
an obsolete `p` in response to such a kill.

BTW, it should also check `ev` in case the event is just that someone
suspended/resumed the background process.


        Stefan

Re: Flymake refactored

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

[sorry if you're getting this twice Stefan, i forgot to cc everyone else]

On Thu, Oct 5, 2017 at 2:11 PM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:
>
> >> :sentinel
> >> (lambda (p _ev)

> > way to do this is to check if 'p' is 'eq' to the buffer-local value of
> > clang-flymake--procress.
>
> While I agree that it should check whether `p` is obsolete (just in case
> something went wrong elsewhere), `p` should have been killed when the
> other process was launched,

I believe the code examples presented here already do that:

>> (when (process-live-p gcc--flymake-proc)
>>    (kill-process gcc--flymake-proc))

> so this sentinel should only be called with an obsolete `p` in response
to such a kill.

But while a "killed" 'p' is obsolete, the reverse isn't true. When lauching
B, the
process A might have already have exited cleanly. Hence A it is not live
anymore, and can't be killed. But A's sentinel might not have had a chance
to run yet. When it does run, A's sentinel must notice that it is obsolete
by
some means, and checking for eqness seems the easiest (in flymake-proc.el
I set an 'obsolete' property in the proc).

> BTW, it should also check `ev` in case the event is just that someone
> suspended/resumed the background process.

That is true.

We should come up with a canonic way to launch flymake processes
and then either hide that behind an abstraction or document it.

João

[-- Attachment #2: Type: text/html, Size: 3293 bytes --]

Re: Flymake refactored

[João Távora]

João Távora <joaotavora@gmail.com> writes:

> We should come up with a canonic way to launch flymake processes
> and then either hide that behind an abstraction or document it.

Attached is a possible such function: it's just like make-process, but
does all those boring checks and has some good defaults. I didn't give
it much testing, but it seems to work and backends become easier to read.

Of course, some macrology can probably get us further, but I wouldn't
want to straitjacket backend writers.

Here's what the Ruby backend looks like after using the diff below my
sig. Notice how no separate variable is needed.

   (defun ruby-flymake (report-fn &rest _args)
     (unless (executable-find
              (car ruby-flymake-command)) (error "Cannot find a suitable ruby"))
     (let ((source (current-buffer)))
       (save-restriction
         (widen)
         (let ((proc
                (flymake-easy-make-process
                 :name "ruby-flymake"
                 :command '("ruby" "-w" "-c")
                 :sentinel
                 (lambda (proc _event)
                   (with-current-buffer (process-buffer proc)
                     (goto-char (point-min))
                     (cl-loop
                      while (search-forward-regexp
                             "^\\(?:.*.rb\\|-\\):\\([0-9]+\\): \\(.*\\)$" nil t)
                      for msg = (match-string 2)
                      for (beg . end) = (flymake-diag-region
                                         source
                                         (string-to-number (match-string 1)))
                      for type = (if (string-match "^warning" msg) :warning :error)
                      collect (flymake-make-diagnostic source beg end type msg)
                      into diags
                      finally (funcall report-fn diags)))))))
           (process-send-region proc (point-min) (point-max))
           (process-send-eof proc)))))

João

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index e747f1a12d..5277e48bc6 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -617,6 +617,82 @@ flymake-make-report-fn
         (with-current-buffer buffer
           (apply #'flymake--handle-report backend token args))))))
 
+(defvar-local flymake-easy-make-process--processes
+  nil)
+
+;;;###autoload
+(defun flymake-easy-make-process (&rest args)
+  "Like `make-process', but suitable for writing Flymake backends.
+Use this function when writing Flymake backends based on invoking
+external processes as it preserves the semantics explained
+
+ARGS is a plist of keyword-value pairs.  The same keywords are
+accepted as in `make-process', but with slightly different
+semantics:
+
+* `:name' is mandatory and `:backend' is a synonym for it.  Can
+  be a string or a symbol and must be constant for the backend,
+  i.e. you mustn't generate a different name every time.
+
+* `:buffer' defaults to a temporary buffer, which is always
+  killed after the process exits.
+
+* `:noquery' defaults to t.
+
+* `:connection-type' defaults to 'pipe'
+
+* `:filter', if provided, is only executed if the process is the
+  most recent process created with `flymake-easy-make-process'
+  for a given buffer and `:name'. If these conditions aren't met,
+  the process is killed immediately.
+
+* `:sentinel', if provided, is only executed if the process has
+  exited cleanly and is the most recent process created with
+  `flymake-easy-make-process' for a given buffer and `:name'."
+  (let* ((backend (or (plist-get args :name)
+                      (plist-get args :backend)
+                      (error "`:name' or `:backend' are mandatory")))
+         (backend (if (symbolp backend) (symbol-name backend) backend))
+         (existing (gethash backend
+                            (or flymake-easy-make-process--processes
+                                (setq flymake-easy-make-process--processes
+                                      (make-hash-table :test #'equal))))))
+    (if (process-live-p existing) (kill-process existing))
+    (setq existing
+          (make-process
+           :name backend
+           :command (plist-get args :command)
+           :buffer (if (plist-member args :buffer)
+                       (plist-get args :buffer)
+                     (generate-new-buffer
+                      (concat " *flymake-easy-make-process-"
+                              backend
+                              "*")))
+           :coding (plist-get args :coding)
+           :noquery (if (plist-member args :noquery)
+                        (plist-get args :noquery)
+                      t)
+           :connection-type (if (plist-member args :connection-type)
+                                (plist-get args :connection-type)
+                              'pipe)
+           :filter (lambda (proc string)
+                     (when (eq proc existing)
+                       (if (plist-member args :filter)
+                           (funcall (plist-get args :filter)
+                                    proc string)
+                         (internal-default-process-filter proc
+                                                          string))))
+           :sentinel (lambda (proc event)
+                       (unwind-protect
+                           (when (and (plist-member args :sentinel)
+                                      (eq proc existing)
+                                      (eq 'exit (process-status proc)))
+                             (funcall (plist-get args :sentinel)
+                                      proc event))
+                         (let ((buf (process-buffer proc)))
+                           (when buf (kill-buffer buf)))))))
+    (puthash backend existing flymake-easy-make-process--processes)))
+
 (defun flymake--collect (fn)
   (let (retval)
     (maphash (lambda (backend state)

Re: Flymake refactored

[Mark Oteiza]

joaotavora@gmail.com (João Távora) writes:

> Mark Oteiza <mvoteiza@udel.edu> writes:
>> I also seem to have problems using the column number (match-string 2)
>> and so it is left unused.
>
> The problem is that you are calling flymake-diag-region in the wrong
> buffer. It has to be the source buffer, so in your case you need a
> (with-current-buffer source ...) around it.
>
> But that inconvenience has been fixed in the very latest code emacs-26
> branch. In that version, you just pass 'source' (the c/c++ buffer) to
> flymake-diag-region.

Ah, thank you.

> I've built a backend very similar to yours but base on gcc (clang is
> 500MB and no time for that right now). Have a look, below my sig.

Nice.  Adding to the pile, a LaTeX checker with chktex below.

> I've also noticed, there's a lot of repetition building up in these
> examples. Later it's probably useful to invent an abstraction that hides
> it away.

At first sight it looks like the much of the sentinel contents could be
abstracted away in something like compilation mode's error regexp alists


;;; flymake-chktex.el --- Flymake backend for chktex(1) -*- lexical-binding: t -*-

;;; Code:

(defgroup flymake-chktex ()
  "Flymake backend for chktex"
  :link '(url-link "http://www.nongnu.org/chktex/")
  :group 'flymake)

(defcustom flymake-chktex-program "chktex"
  "Program."
  :type 'string
  :group 'flymake-chktex)

(defvar-local flymake-chktex--process nil)

(defun flymake-chktex-command (program)
  `(,program "--quiet" "--verbosity=0" "--inputfiles"))

(defun flymake-chktex (report-fn &rest _args)
  (unless (executable-find flymake-chktex-program)
    (error "Cannot find a suitable chktex"))
  (when (process-live-p flymake-chktex--process)
    (kill-process flymake-chktex--process))
  (let ((source (current-buffer)))
    (save-restriction
      (widen)
      (setq flymake-chktex--process
            (make-process
             :name "flymake-chktex"
             :buffer (generate-new-buffer "*flymake-chktex*")
             :command (flymake-chktex-command flymake-chktex-program)
             :noquery t :connection-type 'pipe
             :sentinel
             (lambda (process _event)
               (unwind-protect
                   (when (eq process flymake-chktex--process)
                     (with-current-buffer (process-buffer process)
                       (goto-char (point-min))
                       (cl-loop
                        while (search-forward-regexp
                               "^stdin:\\([0-9]+\\):\\([0-9]+\\):\\([0-9]+\\):\\(.*\\)$"
                               nil t)
                        for msg = (match-string 4)
                        for (beg . end) = (flymake-diag-region source
                                                               (string-to-number (match-string 1))
                                                               (string-to-number (match-string 2)))
                        collect (flymake-make-diagnostic source beg end :warning msg)
                        into diags
                        finally (funcall report-fn diags))))
                 (kill-buffer (process-buffer process))))))
      (process-send-region flymake-chktex--process (point-min) (point-max))
      (process-send-eof flymake-chktex--process))))

(defun flymake-chktex-register ()
  (add-hook 'flymake-diagnostic-functions 'flymake-chktex nil t))

(add-hook 'plain-tex-mode-hook 'flymake-chktex-register)
(add-hook 'latex-mode-hook 'flymake-chktex-register)
(add-hook 'bibtex-mode-hook 'flymake-chktex-register)

(provide 'flymake-chktex)

;;; flymake-chktex.el ends here

Re: Flymake refactored

[João Távora]

Mark Oteiza <mvoteiza@udel.edu> writes:
> joaotavora@gmail.com (João Távora) writes:

> Nice.  Adding to the pile, a LaTeX checker with chktex below.

Keep'em coming. Later we should compile a list of these and decide how
to distribute them. Perhaps they're too late for emacs-26, but not for a
some ELPA "flymake-backends" package.

> At first sight it looks like the much of the sentinel contents could be
> abstracted away in something like compilation mode's error regexp alists

That is a possiblity. It could be used alongside to the
flymake-easy-make-process convenience macro that i proposed in a
parallel thread.

João

Re: Flymake refactored

[Stefan Monnier]

>> joaotavora@gmail.com (João Távora) writes:
>> Nice.  Adding to the pile, a LaTeX checker with chktex below.
> Keep'em coming. Later we should compile a list of these and decide how
> to distribute them. Perhaps they're too late for emacs-26, but not for a
> some ELPA "flymake-backends" package.

I think the Python backend would naturally belong in python.el and the Ruby
backend in ruby-mode.el.
Similarly the LaTeX backend would naturally fit in tex-mode.el.


        Stefan

Re: Flymake refactored

[Lele Gaifax]

[-- Attachment #1: Type: text/plain, Size: 230 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I think the Python backend would naturally belong in python.el

I'm attaching what I'm currently testing out: how should I proceed to
contribute it?

Thanks a lot,
ciao, lele.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: python.el.diff --]
[-- Type: text/x-diff, Size: 3811 bytes --]

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 9aa5134ca0..d29139fbde 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -5137,6 +5137,89 @@ python-util-valid-regexp-p
   (ignore-errors (string-match regexp "") t))
 
 \f
+;;; Flymake integration
+
+(defgroup python-flymake nil
+  "Integration between Python and Flymake."
+  :group 'python
+  :link '(custom-group-link :tag "Flymake" flymake)
+  :version "26.1")
+
+(defcustom python-flymake-command '("pyflakes")
+  "The external tool that will be used to perform the syntax check."
+  :group 'python-flymake
+  :type '(repeat string))
+
+;; The default regexp accomodates for older pyflakes, which did not
+;; report the column number
+(defcustom python-flymake-command-output-regexp
+  "^\\(?:.*\\.py\\|<stdin>\\):\\(?1:[0-9]+\\):\\(?:\\(?2:[0-9]+\\):\\)? \\(?3:.*\\)$"
+  "The regexp used to parse the output of the specified tool.
+It must contain two or three groups: group 1 is the line number, group 2 the
+optional column number and the third is the actual message."
+  :group 'python-flymake
+  :type 'regexp)
+
+(defcustom python-flymake-warn-msg-regexp
+  "\\(^redefinition\\|.*unused.*\\|used$\\)"
+  "The regexp used to discriminate warnings from errors."
+  :group 'python-flymake
+  :type 'regexp)
+
+(defvar-local python--flymake-proc nil)
+
+(defun python-flymake (report-fn &rest _args)
+  (unless (executable-find (car python-flymake-command))
+    (error "Cannot find a suitable checker"))
+
+  (unless (derived-mode-p 'python-mode)
+    (error "Can only work on `python-mode' buffers"))
+
+  (when (process-live-p python--flymake-proc)
+    (kill-process python--flymake-proc))
+
+  (let ((source (current-buffer)))
+    (save-restriction
+      (widen)
+      (setq python--flymake-proc
+            (make-process
+             :name "python-flymake"
+             :noquery t
+             :connection-type 'pipe
+             :buffer (generate-new-buffer " *python-flymake*")
+             :command python-flymake-command
+             :sentinel
+             (lambda (proc _event)
+               (unwind-protect
+                   (when (eq proc python--flymake-proc)
+                     (with-current-buffer (process-buffer proc)
+                       (goto-char (point-min))
+                       (cl-loop
+                        while (search-forward-regexp
+                               python-flymake-command-output-regexp nil t)
+                        for msg = (match-string 3)
+                        for (beg . end) = (flymake-diag-region
+                                           source
+                                           (string-to-number (match-string 1))
+                                           (and (match-string 2)
+                                                (string-to-number
+                                                 (match-string 2))))
+                        for type = (if (string-match
+                                        python-flymake-warn-msg-regexp msg)
+                                       :warning :error)
+                        collect (flymake-make-diagnostic
+                                 source beg end type msg)
+                        into diags
+                        finally (funcall report-fn diags))))
+                 (kill-buffer (process-buffer proc))))))
+      (process-send-region python--flymake-proc (point-min) (point-max))
+      (process-send-eof python--flymake-proc))))
+
+(defun python-flymake-activate ()
+  "Activate the Flymake syntax check on all python-mode buffers."
+  (add-hook 'flymake-diagnostic-functions #'python-flymake nil t))
+
+\f
 (defun python-electric-pair-string-delimiter ()
   (when (and electric-pair-mode
              (memq last-command-event '(?\" ?\'))

[-- Attachment #3: Type: text/plain, Size: 206 bytes --]


-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[Eli Zaretskii]

> From: Lele Gaifax <lele@metapensiero.it>
> Date: Fri, 06 Oct 2017 09:09:46 +0200
> 
> I'm attaching what I'm currently testing out: how should I proceed to
> contribute it?

This is large enough to require legal paperwork.  If you are willing
to assign to the FSF the copyright of your contributions, I will send
you off-list the form to start the paperwork rolling.

Thanks.

Re: Flymake refactored

[Lele Gaifax]

Eli Zaretskii <eliz@gnu.org> writes:

> This is large enough to require legal paperwork.  If you are willing
> to assign to the FSF the copyright of your contributions, I will send
> you off-list the form to start the paperwork rolling.

I should be already enabled for that, as is not my first contribution: or do
you mean that something has changed in the meanwhile and I must redo the
process?

Thanks a lot,
ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[Eli Zaretskii]

> From: Lele Gaifax <lele@metapensiero.it>
> Date: Fri, 06 Oct 2017 10:19:54 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > This is large enough to require legal paperwork.  If you are willing
> > to assign to the FSF the copyright of your contributions, I will send
> > you off-list the form to start the paperwork rolling.
> 
> I should be already enabled for that, as is not my first contribution: or do
> you mean that something has changed in the meanwhile and I must redo the
> process?

You are right, sorry.  I was tricked by the different name that
appears in the copyright assignment.

So this means you should just wait for the patch to be reviewed and
committed by someone with commit rights.

Thanks.

Re: Flymake refactored

[Lele Gaifax]

Eli Zaretskii <eliz@gnu.org> writes:

> You are right, sorry.  I was tricked by the different name that
> appears in the copyright assignment.

No problem at all!

> So this means you should just wait for the patch to be reviewed and
> committed by someone with commit rights.

Great, thanks again!

ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[Mark Oteiza]

Lele Gaifax <lele@metapensiero.it> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> I think the Python backend would naturally belong in python.el
>
> I'm attaching what I'm currently testing out: how should I proceed to
> contribute it?

A small patch on top of it: this facilitates configuring the checker to
instead be flake8, which is a wrapper around a few checkers, including pyflakes

http://flake8.pycqa.org/en/latest/
https://pypi.python.org/pypi/flake8

To use flake8, the command should be '("flake8" "-"), the output regexp

  ^stdin:\(?1:[0-9]+\):\(?:\(?2:[0-9]+\):\)? \(?3:.*\)$

and lifting from flycheck, the alist

  '(("^E9.*$" . :error)
    ("^F82.*$" . :error)
    ("^F83.*$" . :error)
    ("^D.*$" . :info)
    ("^N.*$" . :info))


--- lisp/progmodes/python.el	2017-10-06 08:55:36.328201056 -0400
+++ lisp/progmodes/python2.el	2017-10-06 08:56:18.424137623 -0400
@@ -5160,9 +5160,9 @@
   :group 'python-flymake
   :type 'regexp)
 
-(defcustom python-flymake-warn-msg-regexp
-  "\\(^redefinition\\|.*unused.*\\|used$\\)"
-  "The regexp used to discriminate warnings from errors."
+(defcustom python-flymake-msg-alist
+  '(("\\(^redefinition\\|.*unused.*\\|used$\\)" . :warning))
+  "Alist used to associate messages with their types."
   :group 'python-flymake
   :type 'regexp)
 
@@ -5204,9 +5204,10 @@
                                            (and (match-string 2)
                                                 (string-to-number
                                                  (match-string 2))))
-                        for type = (if (string-match
-                                        python-flymake-warn-msg-regexp msg)
-                                       :warning :error)
+                        for type = (or (assoc-default msg
+                                                      python-flymake-msg-alist
+                                                      #'string-match)
+                                       :error)
                         collect (flymake-make-diagnostic
                                  source beg end type msg)
                         into diags

Re: Flymake refactored

[Lele Gaifax]

Mark Oteiza <mvoteiza@udel.edu> writes:

> A small patch on top of it: this facilitates configuring the checker to
> instead be flake8, which is a wrapper around a few checkers, including
> pyflakes

Yep, I like it to be flexible enough to do that, for example to be able to map
"style issues" reported by the pycodestyle to ":note" entries... so your
msg-alist seems better.

Can you help me on a better definition for the following? In particular, is it
possible to dynamically set the :value-type to allow only one of the keys in
the `flymake-diagnostic-types-alist'?

  (defcustom python-flymake-msg-alist
    '(("\\(^redefinition\\|.*unused.*\\|used$\\)" . :warning))
    "Alist used to associate messages to their types.
  Each element should be a cons-cell (REGEXP . TYPE), where TYPE must be
  one defined in the variable `flymake-diagnostic-types-alist'."
    :group 'python-flymake
    :type '(alist :key-type (regexp) :value-type string))

Thanks&bye, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[Mark Oteiza]

Lele Gaifax <lele@metapensiero.it> writes:

> Mark Oteiza <mvoteiza@udel.edu> writes:
>
>> A small patch on top of it: this facilitates configuring the checker to
>> instead be flake8, which is a wrapper around a few checkers, including
>> pyflakes
>
> Yep, I like it to be flexible enough to do that, for example to be able to map
> "style issues" reported by the pycodestyle to ":note" entries... so your
> msg-alist seems better.
>
> Can you help me on a better definition for the following? In particular, is it
> possible to dynamically set the :value-type to allow only one of the keys in
> the `flymake-diagnostic-types-alist'?
>
>   (defcustom python-flymake-msg-alist
>     '(("\\(^redefinition\\|.*unused.*\\|used$\\)" . :warning))
>     "Alist used to associate messages to their types.
>   Each element should be a cons-cell (REGEXP . TYPE), where TYPE must be
>   one defined in the variable `flymake-diagnostic-types-alist'."
>     :group 'python-flymake
>     :type '(alist :key-type (regexp) :value-type string))

Oops, forgot about the type.  It can probably be

  :value-type (choice (const :error) (const :warning) ...)

Re: Flymake refactored

[Mark Oteiza]

On 06/10/17 at 11:21am, Mark Oteiza wrote:
> Lele Gaifax <lele@metapensiero.it> writes:
> > Can you help me on a better definition for the following? In particular, is it
> > possible to dynamically set the :value-type to allow only one of the keys in
> > the `flymake-diagnostic-types-alist'?
> Oops, forgot about the type.  It can probably be
> 
>   :value-type (choice (const :error) (const :warning) ...)

Sorry, I misunderstood you.  I'm not sure how to do that dynamically
with a custom type.

Re: Flymake refactored

[Lele Gaifax]

Mark Oteiza <mvoteiza@udel.edu> writes:

> Oops, forgot about the type.  It can probably be
>
>   :value-type (choice (const :error) (const :warning) ...)

Ok, something like the following then:

  (defcustom python-flymake-msg-alist
    '(("\\(^redefinition\\|.*unused.*\\|used$\\)" . :warning))
    "Alist used to associate messages to their types.
  Each element should be a cons-cell (REGEXP . TYPE), where TYPE must be
  one defined in the variable `flymake-diagnostic-types-alist'."
    :group 'python-flymake
    :type '(alist :key-type (regexp)
                  :value-type (choice (const :tag "Error" :error)
                                      (const :tag "Warning" :warning)
                                      (const :tag "Note" :note))))

ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[João Távora]

Lele Gaifax <lele@metapensiero.it> writes:
>
>   (defcustom python-flymake-msg-alist
>     '(("\\(^redefinition\\|.*unused.*\\|used$\\)" . :warning))
>     "Alist used to associate messages to their types.
>   Each element should be a cons-cell (REGEXP . TYPE), where TYPE must be
>   one defined in the variable `flymake-diagnostic-types-alist'."
>     :group 'python-flymake
>     :type '(alist :key-type (regexp)
>                   :value-type (choice (const :tag "Error" :error)
>                                       (const :tag "Warning" :warning)
>                                       (const :tag "Note" :note))))

It's more "should be one defined in the variable". If it's not, it's
treated like an error.

Also, it doesn't have to one of the three default types: python-mode can
very well add some new diagnostic type, :python-silly-note or
:super-fatal-error, that extend the built-in ones.

Just a minor comment, in case you weren't seeing this possibility, and
because the :value-type (choice...) apparently excludes it.

João

Re: Flymake refactored

[Lele Gaifax]

joaotavora@gmail.com (João Távora) writes:

> Lele Gaifax <lele@metapensiero.it> writes:
>>
>>   (defcustom python-flymake-msg-alist
>>     '(("\\(^redefinition\\|.*unused.*\\|used$\\)" . :warning))
>>     "Alist used to associate messages to their types.
>>   Each element should be a cons-cell (REGEXP . TYPE), where TYPE must be
>>   one defined in the variable `flymake-diagnostic-types-alist'."
>>     :group 'python-flymake
>>     :type '(alist :key-type (regexp)
>>                   :value-type (choice (const :tag "Error" :error)
>>                                       (const :tag "Warning" :warning)
>>                                       (const :tag "Note" :note))))
>
> It's more "should be one defined in the variable". If it's not, it's
> treated like an error.
>
> Also, it doesn't have to one of the three default types: python-mode can
> very well add some new diagnostic type, :python-silly-note or
> :super-fatal-error, that extend the built-in ones.

Yes indeed. So, what should be ":value-type" set to? A generic `:symbol'?

ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> joaotavora@gmail.com (João Távora) writes:
>>> Nice.  Adding to the pile, a LaTeX checker with chktex below.
>> Keep'em coming. Later we should compile a list of these and decide how
>> to distribute them. Perhaps they're too late for emacs-26, but not for a
>> some ELPA "flymake-backends" package.
>
> I think the Python backend would naturally belong in python.el and the Ruby
> backend in ruby-mode.el.
> Similarly the LaTeX backend would naturally fit in tex-mode.el.

Yes, that is the natural way. But should it go into the emacs-26 branch?
Or rather wait for the rewrite to be merged to master and wait for Emacs
27? In the latter case, I think a package is useful.


João

Re: Flymake refactored

[Stefan Monnier]

> Yes, that is the natural way. But should it go into the emacs-26 branch?

I'll let Eli&John decide.

> Or rather wait for the rewrite to be merged to master and wait for Emacs
> 27?  In the latter case, I think a package is useful.

I merged it to master yesterday, so there should be no need to wait any more.
For python.el there'd be no need for a package since python.el itself is
available via GNU ELPA.  We could do the same for ruby-mode.el.


        Stefan

Re: Flymake refactored

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 07 Oct 2017 09:28:04 -0400
> 
> > Yes, that is the natural way. But should it go into the emacs-26 branch?
> 
> I'll let Eli&John decide.

If it's just a Flymake back-end, and doesn't affect anything else
unless activated, I see no reason not to have it on the release
branch, assuming that its support in the version of Flymake on the
release branch is reasonably good.

Thanks.

Re: Flymake refactored

[Lele Gaifax]

[-- Attachment #1: Type: text/plain, Size: 1971 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Sat, 07 Oct 2017 09:28:04 -0400
>> 
>> > Yes, that is the natural way. But should it go into the emacs-26 branch?
>> 
>> I'll let Eli&John decide.
>
> If it's just a Flymake back-end, and doesn't affect anything else
> unless activated, I see no reason not to have it on the release
> branch, assuming that its support in the version of Flymake on the
> release branch is reasonably good.

Great, I'll do my best to get there: current version of python-flymake,
ameliorated with Noam's suggestions, works pretty well, and I'm right now
trying it with customized settings so that it uses an alternative checker
(flake8).

However, I have a problem when using the new implementation in my real
development context, that wasn't present before (before João work I was using
python-flymake-pyflakes from ELPA).

I use the `desktop' functionality, to keep a persistent state of open files,
and in one project the `emacs.desktop' file has 367 entries, of which 284 are
python-mode buffers: now when I open that project the python-flymake backend
gets disabled with the following errors, for each python-mode buffer:

  Warning [flymake xxx.py]: Disabling backend python-flymake because (file-error Creating pipe Troppi file aperti)
  Warning [flymake xxx.py]: Disabling backend flymake-proc-legacy-flymake because (error Can find a suitable init function)

where "Troppi file aperti" means "Too many open files". The mode-line of all
those buffers tells that Flymake is in "Wait" status.

I wonder if the problem is in the backend or rather in the new Flymake
mechanism. Also, I will try to understand whether there could be a way to tell
Flymake to run the checker only after the buffer has been modified, in other
words to *not* run it on newly open files.

Any hint on how to investigate the issue?

I'm attaching below the current backend I'm trying out.

Thanks&bye, lele.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: python.diff --]
[-- Type: text/x-diff, Size: 4881 bytes --]

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 9aa5134ca0..7328170a9b 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -5137,6 +5137,106 @@ python-util-valid-regexp-p
   (ignore-errors (string-match regexp "") t))
 
 \f
+;;; Flymake integration
+
+(defgroup python-flymake nil
+  "Integration between Python and Flymake."
+  :group 'python
+  :link '(custom-group-link :tag "Flymake" flymake)
+  :version "26.1")
+
+(defcustom python-flymake-command '("pyflakes")
+  "The external tool that will be used to perform the syntax check."
+  :group 'python-flymake
+  :type '(repeat string))
+
+;; The default regexp accomodates for older pyflakes, which did not
+;; report the column number
+(defcustom python-flymake-command-output-regexp
+  "^\\(?:<stdin>\\):\\(?1:[0-9]+\\):\\(?:\\(?2:[0-9]+\\):\\)? \\(?3:.*\\)$"
+  "The regexp used to parse the output of the specified tool.
+It must contain two or three groups: group 1 is the line number, group 2 the
+optional column number and the third is the actual message."
+  :group 'python-flymake
+  :type 'regexp)
+
+(defcustom python-flymake-msg-alist
+  '(("\\(^redefinition\\|.*unused.*\\|used$\\)" . :warning))
+  "Alist used to associate messages to their types.
+Each element should be a cons-cell (REGEXP . TYPE), where TYPE must be
+one defined in the variable `flymake-diagnostic-types-alist'."
+  :group 'python-flymake
+  :type `(alist :key-type (regexp)
+                :value-type (symbol)))
+                ;; :value-type (choice
+                ;;              (mapcar (lambda (type)
+                ;;                        (list 'const
+                ;;                              ':tag
+                ;;                              (capitalize
+                ;;                               (substring
+                ;;                                (symbol-name (car type))
+                ;;                                1 nil))
+                ;;                              (car type)))
+                ;;                      flymake-diagnostic-types-alist))))
+                ;; :value-type (choice (const :tag "Error" :error)
+                ;;                     (const :tag "Warning" :warning)
+                ;;                     (const :tag "Note" :note))))
+
+(defvar-local python--flymake-proc nil)
+
+(defun python-flymake (report-fn &rest _args)
+  (unless (executable-find (car python-flymake-command))
+    (error "Cannot find a suitable checker"))
+
+  (unless (derived-mode-p 'python-mode)
+    (error "Can only work on `python-mode' buffers"))
+
+  (when (process-live-p python--flymake-proc)
+    (kill-process python--flymake-proc))
+
+  (let ((source (current-buffer)))
+    (save-restriction
+      (widen)
+      (setq python--flymake-proc
+            (make-process
+             :name "python-flymake"
+             :noquery t
+             :connection-type 'pipe
+             :buffer (generate-new-buffer " *python-flymake*")
+             :command python-flymake-command
+             :sentinel
+             (lambda (proc _event)
+               (unwind-protect
+                   (when (eq proc python--flymake-proc)
+                     (with-current-buffer (process-buffer proc)
+                       (goto-char (point-min))
+                       (cl-loop
+                        while (search-forward-regexp
+                               python-flymake-command-output-regexp nil t)
+                        for msg = (match-string 3)
+                        for (beg . end) = (flymake-diag-region
+                                           source
+                                           (string-to-number (match-string 1))
+                                           (and (match-string 2)
+                                                (string-to-number
+                                                 (match-string 2))))
+                        for type = (or (assoc-default msg
+                                                      python-flymake-msg-alist
+                                                      #'string-match)
+                                       :error)
+                        collect (flymake-make-diagnostic
+                                 source beg end type msg)
+                        into diags
+                        finally (funcall report-fn diags))))
+                 (kill-buffer (process-buffer proc))))))
+      (process-send-region python--flymake-proc (point-min) (point-max))
+      (process-send-eof python--flymake-proc))))
+
+(defun python-flymake-activate ()
+  "Activate the Flymake syntax check on all python-mode buffers."
+  (add-hook 'flymake-diagnostic-functions #'python-flymake nil t))
+
+\f
 (defun python-electric-pair-string-delimiter ()
   (when (and electric-pair-mode
              (memq last-command-event '(?\" ?\'))

[-- Attachment #3: Type: text/plain, Size: 206 bytes --]


-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[Eli Zaretskii]

> From: Lele Gaifax <lele@metapensiero.it>
> Date: Sat, 07 Oct 2017 16:40:11 +0200
> 
>   Warning [flymake xxx.py]: Disabling backend python-flymake because (file-error Creating pipe Troppi file aperti)
>   Warning [flymake xxx.py]: Disabling backend flymake-proc-legacy-flymake because (error Can find a suitable init function)
> 
> where "Troppi file aperti" means "Too many open files". The mode-line of all
> those buffers tells that Flymake is in "Wait" status.
> 
> I wonder if the problem is in the backend or rather in the new Flymake
> mechanism. Also, I will try to understand whether there could be a way to tell
> Flymake to run the checker only after the buffer has been modified, in other
> words to *not* run it on newly open files.
> 
> Any hint on how to investigate the issue?

How about simply disabling this during desktop-restore?

Re: Flymake refactored

[Stefan Monnier]

> However, I have a problem when using the new implementation in my real
> development context, that wasn't present before (before João work I was using
> python-flymake-pyflakes from ELPA).

> I use the `desktop' functionality, to keep a persistent state of open files,
> and in one project the `emacs.desktop' file has 367 entries, of which 284 are
> python-mode buffers: now when I open that project the python-flymake backend
> gets disabled with the following errors, for each python-mode buffer:

>   Warning [flymake xxx.py]: Disabling backend python-flymake because
> (file-error Creating pipe Troppi file aperti)

IIUC this problem is with some "old" backend that worked fine with the
old flymake but now exhibits a poor behavior with the new flymake.
In that case I suggest you `M-x report-emacs-bug` so we can track
it down.

It's probably due to changes in the way flymake decides when to run
the backend(s) and we should fix those issues.


        Stefan

Re: Flymake refactored

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> However, I have a problem when using the new implementation in my real
>> development context, that wasn't present before (before João work I was using
>> python-flymake-pyflakes from ELPA).
>
>> I use the `desktop' functionality, to keep a persistent state of open files,
>> and in one project the `emacs.desktop' file has 367 entries, of which 284 are
>> python-mode buffers: now when I open that project the python-flymake backend
>> gets disabled with the following errors, for each python-mode buffer:
>
>>   Warning [flymake xxx.py]: Disabling backend python-flymake because
>> (file-error Creating pipe Troppi file aperti)
>
> IIUC this problem is with some "old" backend that worked fine with the
> old flymake but now exhibits a poor behavior with the new flymake.
> In that case I suggest you `M-x report-emacs-bug` so we can track
> it down.

A seemingly good workaround to fix this is to bind
flymake-start-syntax-check-on-find-file to nil around that mega
file-finding operation. This is a bit what Eli suggested, I think.

And I think the only real reason "old" Flymake managed to launch ~300
processes immediately without choking up is that it did so under
with-demoted-errors, so that's not really a fair comparison (though,
granted, I removed it --- and now I understand why it was there).

> It's probably due to changes in the way flymake decides when to run
> the backend(s) and we should fix those issues.

I don't see what flymake.el can do about it, since it is was designed to
be agnostic to the way backends allocate resources to start syntax
checks.

In other words, to "properly" fix this, the Python backend must throttle
its process-making, which is another argument in favor of using a more
sophisticated version of the 'flymake-easy-make-process' function that I
sent you some a couple of days ago.

Another way to fix it is to add throttling capabilities to make-process
itself.  I don't know if it is possible since it is a big change to its
semantics, which seem to imply that system resources are immediately
allocated. Meaning that if a :throttle arg were accepted

   (process-id (make-process :name "true" :command '("true") :throttle t))

wouldn't make sense, because make-process could actually not have
allocated any system resources.

Re: Flymake refactored

[Lele Gaifax]

joaotavora@gmail.com (João Távora) writes:

> A seemingly good workaround to fix this is to bind
> flymake-start-syntax-check-on-find-file to nil around that mega
> file-finding operation. This is a bit what Eli suggested, I think.

Thanks, that made the job! After trying with the temporary bind around
`desktop-read' (that works), I opted to leave that setting to nil, so that I
get the diagnostics only for the buffers I actually edit: this lowers the
chance I get distracted and compulsively start to fix style issues reported by
pycodestyle :-)

ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[Stefan Monnier]

> And I think the only real reason "old" Flymake managed to launch ~300
> processes immediately without choking up is that it did so under
> with-demoted-errors, so that's not really a fair comparison (though,
> granted, I removed it --- and now I understand why it was there).

Ah, in which case it's not that the old flymake.el worked, but that it
croaked more discretely?  That would be a good explanation.

> I don't see what flymake.el can do about it, since it is was designed
> to be agnostic to the way backends allocate resources to start
> syntax checks.

The problem can be fixed "anywhere" between desktop.el and make-process.

There's a good argument that desktop.el should load buffers more lazily.

There's also a good argument to be made that flymake.el should itself work
more lazily (don't start checking until the buffer is actually
displayed, for example).

I think asking backends to perform the throttling is a bad idea: we want
the backends to be as lean/simple/concise as possible.

Performing the throttling in make-process is also an option, but I think
it's probably too low a level to do it right, unless we add a :throttle
arg like you suggest (so that the higher-level code can pass the info),
but that pushes the responsibility to the backend again.


        Stefan

Re: Flymake refactored

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Ah, in which case it's not that the old flymake.el worked, but that it
> croaked more discretely?  That would be a good explanation.

Yes that's it. And "croaked" is a really funny word :-)

>> I don't see what flymake.el can do about it, since it is was designed
>> to be agnostic to the way backends allocate resources to start
>> syntax checks.
>
> The problem can be fixed "anywhere" between desktop.el and make-process.
>
> There's a good argument that desktop.el should load buffers more lazily.

Would be hard to predict how lazy it has to be fix these issues.

> There's also a good argument to be made that flymake.el should itself work
> more lazily (don't start checking until the buffer is actually
> displayed, for example).

I like this one. So flymake-mode either starts the check or does
something to window-configuration-change-hook? Is that the idea?

> I think asking backends to perform the throttling is a bad idea: we want
> the backends to be as lean/simple/concise as possible.

If they used the make-process helper, they would keep these
qualities. But on second thought the whole idea behind throttling
(make-process or backend) seems too feeble to be worth the effort. It
only works cooperatively, if all process creation uses it. If it is
optional, it's not worth it.

Re: Flymake refactored

[Stefan Monnier]

>> The problem can be fixed "anywhere" between desktop.el and make-process.
>> There's a good argument that desktop.el should load buffers more lazily.
> Would be hard to predict how lazy it has to be fix these issues.

The same kind of issues appears with large desktop sessions w.r.t other
features than flymake.  We had a discussion here some months ago
(year(s)?) about having desktop create "uninitialized buffers" which
only finish their initialization when they get displayed.

So it would behave a bit like what happens with previously "saved tabs"
when you restart Firefox.

>> There's also a good argument to be made that flymake.el should itself work
>> more lazily (don't start checking until the buffer is actually
>> displayed, for example).
> I like this one.  So flymake-mode either starts the check or does
> something to window-configuration-change-hook?  Is that the idea?

I hadn't thought about how to go about it, but indeed maybe
window-configuration-change-hook could do the trick.

>> I think asking backends to perform the throttling is a bad idea: we want
>> the backends to be as lean/simple/concise as possible.
> If they used the make-process helper, they would keep these
> qualities. But on second thought the whole idea behind throttling
> (make-process or backend) seems too feeble to be worth the effort. It
> only works cooperatively, if all process creation uses it. If it is
> optional, it's not worth it.

There's also the issue that you end up checking all those many buffers
(i.e. consuming CPU&RAM&battery during this time) without any guarantee
that we'll actually look at those buffers.


        Stefan

Re: Flymake refactored

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> There's also a good argument to be made that flymake.el should itself work
>>> more lazily (don't start checking until the buffer is actually
>>> displayed, for example).
>> I like this one.  So flymake-mode either starts the check or does
>> something to window-configuration-change-hook?  Is that the idea?
> I hadn't thought about how to go about it, but indeed maybe
> window-configuration-change-hook could do the trick.

I think it does the trick nicely. I just pushed 11b37b4a9, please have a
look. Every `flymake-start' is deferred now to waiting until after the
current command is over (if that command exists) and until the buffer is
displayed (if it is not already). I think this is The Right Thing, as it
also takes care of editing and save operations in undisplayed buffers.

>> only works cooperatively, if all process creation uses it. If it is
>> optional, it's not worth it.
> There's also the issue that you end up checking all those many buffers
> (i.e. consuming CPU&RAM&battery during this time) without any guarantee
> that we'll actually look at those buffers.

This is an orthogonal bit taken care by the above fix. I agree it mostly
render this throttling sophistication useless.

[SUSPECTED SPAM] Re: Flymake refactored

[Stefan Monnier]

>> I hadn't thought about how to go about it, but indeed maybe
>> window-configuration-change-hook could do the trick.
> I think it does the trick nicely. I just pushed 11b37b4a9, please have a
> look.

Looks great, thanks.


        Stefan

[PATCH] Re: Flymake refactored

[Lele Gaifax]

joaotavora@gmail.com (João Távora) writes:

> I think it does the trick nicely. I just pushed 11b37b4a9, please have a
> look.

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 27ec7a1f12..71079681d6 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -124,7 +124,7 @@ flymake-gui-warnings-enabled
                        "it no longer has any effect." "26.1")
 
 (defcustom flymake-start-on-flymake-mode t
-  "Start syntax check when `pflymake-mode'is enabled.
+  "Start syntax check when `flymake-mode' is enabled.
 Specifically, start it when the buffer is actually displayed."
   :type 'boolean)
 
Thanks&bye, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[Marcin Borkowski]

On 2017-10-06, at 05:35, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Similarly the LaTeX backend would naturally fit in tex-mode.el.

Please no.  What about AUCTeX users?

-- 
Marcin Borkowski

Re: Flymake refactored

[Stefan Monnier]

>> Similarly the LaTeX backend would naturally fit in tex-mode.el.
> Please no.  What about AUCTeX users?

AUCTeX can (require 'tex-mode) and then use that function.

AUCTeX and tex-mode should share more code anyway (IIUC they already
share font-lock rules, at least to the extent that you can use tex-mode
font-lock rules in AUCTeX, tho AUCTeX also comes with its own font-lock
rules that you can use instead).


        Stefan

Re: Flymake refactored

[Marcin Borkowski]

On 2017-10-07, at 15:37, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>>> Similarly the LaTeX backend would naturally fit in tex-mode.el.
>> Please no.  What about AUCTeX users?
>
> AUCTeX can (require 'tex-mode) and then use that function.
>
> AUCTeX and tex-mode should share more code anyway (IIUC they already
> share font-lock rules, at least to the extent that you can use tex-mode
> font-lock rules in AUCTeX, tho AUCTeX also comes with its own font-lock
> rules that you can use instead).

OK, I didn't know that.

Best,

-- 
Marcin Borkowski

Re: Flymake refactored

[John Wiegley]

>>>>> "MO" == Mark Oteiza <mvoteiza@udel.edu> writes:

MO> Nice. Adding to the pile, a LaTeX checker with chktex below.

Should we take a look at the list of checkers provided by flycheck, for ideas?

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Flymake refactored

[Mark Oteiza]

On 06/10/17 at 08:54am, John Wiegley wrote:
> >>>>> "MO" == Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> MO> Nice. Adding to the pile, a LaTeX checker with chktex below.
> 
> Should we take a look at the list of checkers provided by flycheck, for ideas?

Sure, I've been reading flycheck and syntastic (analogous package for
vim) for reference.

There are some things aside from checkers I think flymake should learn
from flycheck--may as well list some here:

- some way (global variable?) to disable checkers.  I for one never want
  checkdoc to run automatically
- fine control over when checks happen (again a global setting);
  for instance, on-the-fly can be troublesome if checking is expensive.
  flycheck uses a list: '(save idle-change new-line mode-enabled)
- popup a special buffer with all the error/warning/info listed

Re: Flymake refactored

[João Távora]

Mark Oteiza <mvoteiza@udel.edu> writes:

> - some way (global variable?) to disable checkers.  I for one never want
>   checkdoc to run automatically

Are add/remove-hook enough?

    (add-hook 'emacs-lisp-mode
       (lambda () (remove-hook 'flymake-diagnostic-functions
                               'elisp-flymake-checkdoc t)))

(Perhaps checkdoc shouldn't be in the default, indeed)

> - fine control over when checks happen (again a global setting);
>   for instance, on-the-fly can be troublesome if checking is expensive.
>   flycheck uses a list: '(save idle-change new-line mode-enabled)

Global or per-checker? If global, you already have some:

* flymake-start-syntax-check-on-newline
* flymake-no-changes-timeout (set to nil to disable automatic idle-checking)
* flymake-start-syntax-check-on-find-file

The "on save" behaviour isn't very easy to configure yet.  The names
aren't brilliant, they're inherited from old Flymake.

> - popup a special buffer with all the error/warning/info listed

I really like that one too, and it seems easy enough to do, but we
should also think about the the next-error thingy.

Re: Flymake refactored

[Mark Oteiza]

On 06/10/17 at 05:04pm, João Távora wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > - some way (global variable?) to disable checkers.  I for one never want
> >   checkdoc to run automatically
> 
> Are add/remove-hook enough?
> 
>     (add-hook 'emacs-lisp-mode
>        (lambda () (remove-hook 'flymake-diagnostic-functions
>                                'elisp-flymake-checkdoc t)))

It's enough, but it might be nicer to have a single place to blacklist
things:

  (setq flymake-diagnostic-blacklist '(elisp-flymake-checkdoc ...))

It somewhat begs the question whether checkers should be registered
buffer locally or instead put into the global value of
flymake-diagnostic-functions and written like

  (defun gcc-flymake (report-fn &rest _args)
    (when (or (derived-mode-p 'c-mode) (derived-mode-p 'c++-mode))
      (unless (executable-find gcc-program)
        (error "Cannot find a suitable gcc"))
      ...))

but perhaps there is a downside to this beyond possibly needlessly doing
a lot of derived-mode-p checks while running through the hook.

> > - fine control over when checks happen (again a global setting);
> >   for instance, on-the-fly can be troublesome if checking is expensive.
> >   flycheck uses a list: '(save idle-change new-line mode-enabled)
> 
> Global or per-checker? If global, you already have some:
> 
> * flymake-start-syntax-check-on-newline
> * flymake-no-changes-timeout (set to nil to disable automatic idle-checking)
> * flymake-start-syntax-check-on-find-file
> 
> The "on save" behaviour isn't very easy to configure yet.  The names
> aren't brilliant, they're inherited from old Flymake.

Oh ok, thanks.  I missed the latter two variables.

> > - popup a special buffer with all the error/warning/info listed
> 
> I really like that one too, and it seems easy enough to do, but we
> should also think about the the next-error thingy.

Re: Flymake refactored

[João Távora]

Mark Oteiza <mvoteiza@udel.edu> writes:

> but perhaps there is a downside to this beyond possibly needlessly doing
> a lot of derived-mode-p checks while running through the hook.

Yes, it's repeating what mode hooks are supposed to take care of.
Stefan and I discussed it during the initial review.  In the end he
convinced me, and I removed the derived-mode-p in elisp-mode.el

But I can understand the akwardness you are experiencing. It is because
you are forced to use hooks inside hooks.  

A global blacklist is less bizarre but still confusing if you just know
about flymake-diagnostic-functions.  What about taking advantage of the
fact that erroring backends disable themselves with this fun hack?

   (defun blacklist (&rest _args) (error "blacklisted"))
    
   (advice-add 'elisp-flymake-checkdoc :around 'blacklist)
   (advice-remove 'elisp-flymake-checkdoc 'blacklist) ; regret it


>> * flymake-no-changes-timeout (set to nil to disable automatic idle-checking)
>> * flymake-start-syntax-check-on-find-file
>> 
>> The "on save" behaviour isn't very easy to configure yet.  The names
>> aren't brilliant, they're inherited from old Flymake.
>
> Oh ok, thanks.  I missed the latter two variables.

I thought you were going to ask for per-backend values. In that
hypothetical case, then I guess these varaibles could also take on
functions of a single argument as values. The argument would be the
backend and the return value would be used. Kinda tricky (but not
impossible) for flymake-no-changes-timeout though (perhaps only
nil/non-nil should be allowed for that one.)

João

Re: Flymake refactored

[Stefan Monnier]

> - some way (global variable?) to disable checkers.  I for one never want
>   checkdoc to run automatically

FWIW, I partly agree, but I think it's only because checkdoc has too
many false positives and/or too stringent requirements (e.g. that all
args be documented).  So I think the better way to fix this particular
issue is to improve/tweak checkdoc.


        Stefan

Re: Flymake refactored

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> - some way (global variable?) to disable checkers.  I for one never want
>>   checkdoc to run automatically
>
> FWIW, I partly agree, but I think it's only because checkdoc has too
> many false positives and/or too stringent requirements (e.g. that all
> args be documented).  So I think the better way to fix this particular
> issue is to improve/tweak checkdoc.

+1, some checkdoc-stringency variable that elisp-mode's use of checkdoc
can let-bind to 'reasonable.

João

Re: Flymake refactored

[João Távora]

Mark Oteiza <mvoteiza@udel.edu> writes:

> Sure, I've been reading flycheck and syntastic (analogous package for
> vim) for reference.
>
> There are some things aside from checkers I think flymake should learn
> from flycheck--may as well list some here:
> [...]
> - popup a special buffer with all the error/warning/info listed

Please have a look at the scratch/flymake-diagnostics-buffer branch and
tell me what you think (perhaps comparing it to Flycheck's). The command
is flymake-show-diagnostics-buffer.

It's very naively implemented for now (and seems kinda slow).

João

Re: Flymake refactored

[Mark Oteiza]

On 07/10/17 at 05:07pm, João Távora wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > Sure, I've been reading flycheck and syntastic (analogous package for
> > vim) for reference.
> >
> > There are some things aside from checkers I think flymake should learn
> > from flycheck--may as well list some here:
> > [...]
> > - popup a special buffer with all the error/warning/info listed
> 
> Please have a look at the scratch/flymake-diagnostics-buffer branch and
> tell me what you think (perhaps comparing it to Flycheck's). The command
> is flymake-show-diagnostics-buffer.
> 
> It's very naively implemented for now (and seems kinda slow).

Looks good, I'd just change from using buttons to having the whole line
be usable to navigate to the error.  Perhaps the biggest thing is doing
like M-x grep and being able to M-g M-{n,p} and follow in the code
buffer, but I suspect that ties into the next-error issue.

I suspect it's the use of overlays making it slow--I don't think you
need overlays at all for this--just store what you need in the
tabulated-list id which IIRC gets applied to the whole line as a text
property, which you can then use with (tabulated-list-get-id)

Re: Flymake refactored

[João Távora]

Mark Oteiza <mvoteiza@udel.edu> writes:

> On 07/10/17 at 05:07pm, João Távora wrote:
>> Mark Oteiza <mvoteiza@udel.edu> writes:
>> 
>> > Sure, I've been reading flycheck and syntastic (analogous package for
>> > vim) for reference.
>> >
>> > There are some things aside from checkers I think flymake should learn
>> > from flycheck--may as well list some here:
>> > [...]
>> > - popup a special buffer with all the error/warning/info listed
>> 
>> Please have a look at the scratch/flymake-diagnostics-buffer branch and
>> tell me what you think (perhaps comparing it to Flycheck's). The command
>> is flymake-show-diagnostics-buffer.
>> 
>> It's very naively implemented for now (and seems kinda slow).
>
> Looks good, I'd just change from using buttons to having the whole line
> be usable to navigate to the error.

Agree, makes sense. But this seems akward to do in
tabulated-list-mode. I don't mind if you beat me to it :-)

> Perhaps the biggest thing is doing
> like M-x grep and being able to M-g M-{n,p} and follow in the code
> buffer, but I suspect that ties into the next-error issue.

Yes, I believe this can of worms has to be opened eventually.

> I suspect it's the use of overlays making it slow--I don't think you
> need overlays at all for this--just store what you need in the
> tabulated-list id which IIRC gets applied to the whole line as a text
> property, which you can then use with (tabulated-list-get-id)

But it doesn't make any new overlays, if that was your idea. The
overlays used are the ones in the source buffer, where they can hardly
be avoided. Using them here seemed like the easiest and fastest way to
get to all Flymake diagnostics in a buffer.

I might have exaggerated the performance hit, since it doesn't seem so
slow to me now. Perhaps we could get some big files full of errors and
run some benchmarks with a proper backend that can be found in Flycheck,
too.

João

Re: Flymake refactored

[Mark Oteiza]

On 08/10/17 at 10:06am, João Távora wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > On 07/10/17 at 05:07pm, João Távora wrote:
> >> Mark Oteiza <mvoteiza@udel.edu> writes:
> >> 
> >> > Sure, I've been reading flycheck and syntastic (analogous package for
> >> > vim) for reference.
> >> >
> >> > There are some things aside from checkers I think flymake should learn
> >> > from flycheck--may as well list some here:
> >> > [...]
> >> > - popup a special buffer with all the error/warning/info listed
> >> 
> >> Please have a look at the scratch/flymake-diagnostics-buffer branch and
> >> tell me what you think (perhaps comparing it to Flycheck's). The command
> >> is flymake-show-diagnostics-buffer.
> >> 
> >> It's very naively implemented for now (and seems kinda slow).
> >
> > Looks good, I'd just change from using buttons to having the whole line
> > be usable to navigate to the error.
> 
> Agree, makes sense. But this seems akward to do in
> tabulated-list-mode. I don't mind if you beat me to it :-)

Patch below :)

> > I suspect it's the use of overlays making it slow--I don't think you
> > need overlays at all for this--just store what you need in the
> > tabulated-list id which IIRC gets applied to the whole line as a text
> > property, which you can then use with (tabulated-list-get-id)
> 
> But it doesn't make any new overlays, if that was your idea. The
> overlays used are the ones in the source buffer, where they can hardly
> be avoided. Using them here seemed like the easiest and fastest way to
> get to all Flymake diagnostics in a buffer.

Oh true, my mistake.

> I might have exaggerated the performance hit, since it doesn't seem so
> slow to me now. Perhaps we could get some big files full of errors and
> run some benchmarks with a proper backend that can be found in Flycheck,
> too.

Probably overkill, but my best example is the Freefem++ manual: an 860
kB, 21k-line .tex file which triggers thousands of warnings in
chktex(1).

It took about ten minutes for flycheck to check it (vs a few seconds to
make the diagnostic list), and you have to set
flycheck-checker-error-threshold to nil in order to prevent flycheck
from disabling the checker.  This is one thing I _don't_ like about
flycheck--I would like the ability to at least truncate results.

It doesn't look like they have a browsable repository.  You'll find it
as DOC/freefem++doc.tex in the latest tarball.
http://www.freefem.org/ff++/ftp/freefem++-3.56-1.tar.gz

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 24b1950c1a..fb5fc7db12 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -988,17 +988,19 @@ flymake--mode-line-format
 
 (defvar-local flymake--diagnostics-buffer-source nil)
 
-(defvar flymake--diagnostics-buffer-button-keymap
+(defvar flymake-diagnostics-buffer-mode-map
   (let ((map (make-sparse-keymap)))
-    (define-key map [mouse-1] 'push-button)
-    (define-key map (kbd "RET") 'push-button)
+    (define-key map [mouse-1] 'flymake-goto-diagnostic-at-point)
+    (define-key map (kbd "RET") 'flymake-goto-diagnostic-at-point)
     (define-key map (kbd "SPC") 'flymake-show-diagnostic-at-point)
     map))
 
-(defun flymake-show-diagnostic-at-point (button)
-  "Show location of diagnostic of BUTTON."
-  (interactive (list (button-at (point))))
-  (let* ((overlay (button-get button 'flymake-overlay)))
+(defun flymake-show-diagnostic-at-point ()
+  "Show location of diagnostic at point."
+  (interactive)
+  (let* ((id (or (tabulated-list-get-id)
+                 (user-error "Nothing at point")))
+         (overlay (plist-get id :overlay)))
     (with-current-buffer (overlay-buffer overlay)
       (with-selected-window
           (display-buffer (current-buffer))
@@ -1008,11 +1010,11 @@ flymake-show-diagnostic-at-point
                                           'highlight))
       (current-buffer))))
 
-(defun flymake-goto-diagnostic-at-point (button)
-  "Show location of diagnostic of BUTTON."
-  (interactive (list (button-at (point))))
+(defun flymake-goto-diagnostic-at-point ()
+  "Show location of diagnostic at point."
+  (interactive)
   (pop-to-buffer
-   (flymake-show-diagnostic-at-point button)))
+   (flymake-show-diagnostic-at-point)))
 
 (defun flymake--diagnostics-buffer-entries ()
   (with-current-buffer flymake--diagnostics-buffer-source
@@ -1032,16 +1034,7 @@ flymake--diagnostics-buffer-entries
                          :severity (flymake--lookup-type-property
                                     type
                                     'severity (warning-numeric-level :error)))
-                   `[(,(format "%s" line)
-                      keymap ,flymake--diagnostics-buffer-button-keymap
-                      action flymake-goto-diagnostic-at-point
-                      mouse-action flymake-goto-diagnostic-at-point
-                      help-echo ,(mapconcat #'identity
-                                            '("mouse-1, RET: goto location at point"
-                                              "SPC: show location at point")
-                                            "\n")
-                      flymake-diagnostic ,diag
-                      flymake-overlay ,ov)
+                   `[,(format "%s" line)
                      ,(format "%s" col)
                      ,(propertize (format "%s" type)
                                   'face (flymake--lookup-type-property

Re: Flymake refactored

[João Távora]

Mark Oteiza <mvoteiza@udel.edu> writes:

>> Agree, makes sense. But this seems akward to do in
>> tabulated-list-mode. I don't mind if you beat me to it :-)
>
> Patch below :)

Thanks, applied it. But tweaked it again (patch below) to at least have
mouse-face 'highlight on the message when hovering on it. This is
slightly more complicated but more in line with what the Elisp manual
node seems to suggest doing in "Clickable Text" (there seems to be more
than one way tho).

Feel free to tweak again and push to the
scratch/flymake-diagnostics-buffer branch.

> Probably overkill, but my best example is the Freefem++ manual: an 860

Yes, 10 minutes is overkill, but we can probably scale it to 10% or
so. Unfortunately, I don't have time to work on this properly now, so
don't hold back :-)

João

commit 9412afa5f601f0d3f6d6d094bf5b918a41a3e136
Author: João Távora <joaotavora@gmail.com>
Date:   Mon Oct 9 00:12:48 2017 +0100

    Tweak the Flymake diagnostics buffer again
    
    * lisp/progmodes/flymake.el
    (flymake-diagnostics-buffer-mode-map): Don't bind [mouse-1].
    (flymake-show-diagnostic): Rename from
    flymake-show-diagnostic-at-point.  Really use another window.
    (flymake-goto-diagnostic): Rename from
    flymake-goto-diagnostic-at-point.
    (flymake--diagnostics-buffer-entries): Use a button just for
    the message bit.

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index fb5fc7db12..6796fc2b76 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -990,31 +990,31 @@ flymake--diagnostics-buffer-source
 
 (defvar flymake-diagnostics-buffer-mode-map
   (let ((map (make-sparse-keymap)))
-    (define-key map [mouse-1] 'flymake-goto-diagnostic-at-point)
-    (define-key map (kbd "RET") 'flymake-goto-diagnostic-at-point)
-    (define-key map (kbd "SPC") 'flymake-show-diagnostic-at-point)
+    (define-key map (kbd "RET") 'flymake-goto-diagnostic)
+    (define-key map (kbd "SPC") 'flymake-show-diagnostic)
     map))
 
-(defun flymake-show-diagnostic-at-point ()
-  "Show location of diagnostic at point."
-  (interactive)
-  (let* ((id (or (tabulated-list-get-id)
+(defun flymake-show-diagnostic (pos &optional other-window)
+  "Show location of diagnostic at POS."
+  (interactive (list (point) t))
+  (let* ((id (or (tabulated-list-get-id pos)
                  (user-error "Nothing at point")))
          (overlay (plist-get id :overlay)))
     (with-current-buffer (overlay-buffer overlay)
       (with-selected-window
-          (display-buffer (current-buffer))
+          (display-buffer (current-buffer) other-window)
         (goto-char (overlay-start overlay))
         (pulse-momentary-highlight-region (overlay-start overlay)
                                           (overlay-end overlay)
                                           'highlight))
       (current-buffer))))
 
-(defun flymake-goto-diagnostic-at-point ()
-  "Show location of diagnostic at point."
-  (interactive)
+(defun flymake-goto-diagnostic (pos)
+  "Show location of diagnostic at POS.
+POS can be a buffer position or a button"
+  (interactive "d")
   (pop-to-buffer
-   (flymake-show-diagnostic-at-point)))
+   (flymake-show-diagnostic (if (button-type pos) (button-start pos) pos))))
 
 (defun flymake--diagnostics-buffer-entries ()
   (with-current-buffer flymake--diagnostics-buffer-source
@@ -1039,7 +1039,11 @@ flymake--diagnostics-buffer-entries
                      ,(propertize (format "%s" type)
                                   'face (flymake--lookup-type-property
                                          type 'mode-line-face 'flymake-error))
-                     ,(format "%s" (flymake--diag-text diag))]))))
+                     (,(format "%s" (flymake--diag-text diag))
+                      mouse-face highlight
+                      help-echo "mouse-2: visit this diagnostic"
+                      face nil
+                      mouse-action flymake-goto-diagnostic)]))))
 
 (define-derived-mode flymake-diagnostics-buffer-mode tabulated-list-mode
   "Flymake diagnostics"

Re: Flymake refactored

[Mark Oteiza]

On 09/10/17 at 12:21am, João Távora wrote:
> Feel free to tweak again and push to the
> scratch/flymake-diagnostics-buffer branch.

I just added the keymap to the button--did not notice anything else
wrong with it.

Re: Flymake refactored

[João Távora]

Mark Oteiza <mvoteiza@udel.edu> writes:

> On 09/10/17 at 12:21am, João Távora wrote:
>> Feel free to tweak again and push to the
>> scratch/flymake-diagnostics-buffer branch.
>
> I just added the keymap to the button--did not notice anything else
> wrong with it.

Remarkably, this breaks the button (at least in my testing) for
mouse-clicks, which is the only reason it is there. Being a button I
think one shouldn't mess with its keymap.

Rather, this is what is needed unbreak RET on the button (and still keep
mouse-1, mouse-2 and SPC, naturally)

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index cf1e7e41ba..e4c6a38a77 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -1132,7 +1132,7 @@ flymake--diagnostics-buffer-entries
                       mouse-face highlight
                       help-echo "mouse-2: visit this diagnostic"
                       face nil
-                      keymap flymake-diagnostics-buffer-mode-map
+                      action flymake-goto-diagnostic
                       mouse-action flymake-goto-diagnostic)]))))
 
 (define-derived-mode flymake-diagnostics-buffer-mode tabulated-list-mode

João

Re: Flymake refactored

[Mark Oteiza]

On 10/10/17 at 04:20pm, João Távora wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > On 09/10/17 at 12:21am, João Távora wrote:
> >> Feel free to tweak again and push to the
> >> scratch/flymake-diagnostics-buffer branch.
> >
> > I just added the keymap to the button--did not notice anything else
> > wrong with it.
> 
> Remarkably, this breaks the button (at least in my testing) for
> mouse-clicks, which is the only reason it is there. Being a button I
> think one shouldn't mess with its keymap.
> 
> Rather, this is what is needed unbreak RET on the button (and still keep
> mouse-1, mouse-2 and SPC, naturally)

Agreed, my mistake.

Re: Flymake refactored

[Lele Gaifax]

joaotavora@gmail.com (João Távora) writes:

> For example, here's a decent Ruby checker under 40 lines that does the
> same as MELPA's flymake-ruby.el (which is based on flymake-easy), but
> using the new API and without creating any temporary files.

Thank you João for the example, I'm attaching below my Python pyflakes
variant, that seems working pretty well.

I have a couple of questions:

a) I had to use lexical-binding, otherwise the funcall form in the inner
   lambda raises an error about report-fn being undefined; did I miss
   something, or is that the right thing to do?

b) I had to omit the "local" flag when hooking esk/python-flymake to
   flymake-diagnostic-functions as you did in the example: shouldn't the
   latter be a defvar-local variable?

Thanks in advance,
ciao, lele.



;; excerpt from my .emacs.d/esk/python.el -- -*- lexical-binding:t -*-
;;

;; TODO: Maybe defcustom the following two?
(defvar esk/python-flymake-command '("python3.6" "-m" "pyflakes"))
(defvar esk/python-flymake-warning-regexp "\\(^redefinition\\|.*unused.*\\|used$\\)")

(defvar-local esk/python--flymake-proc nil)

;; Explicitly use Python 3.6, to handle f-strings
(defvar esk/python-flymake-command '("python3.6" "-m" "pyflakes"))

;; Accomodate for older pyflakes, which did not report the column number
(defvar esk/python-flymake-diag-regexp
  "^\\(?:.*\\.p[yj]\\|<stdin>\\):\\([0-9]+\\):\\(?:\\([0-9]+\\):\\)? \\(.*\\)$")

;; Recognize warning messages
(defvar esk/python-flymake-warn-msg-regexp "\\(^redefinition\\|.*unused.*\\|used$\\)")

(defvar-local esk/python--flymake-proc nil)

(defun esk/python-flymake (report-fn &rest _args)
  (unless (executable-find (car esk/python-flymake-command))
    (error "Cannot find a suitable checker"))

  (unless (derived-mode-p 'python-mode)
    (error "Can only work on `python-mode' buffers"))

  (when (process-live-p esk/python--flymake-proc)
    (kill-process esk/python--flymake-proc))

  (let ((source (current-buffer)))
    (save-restriction
      (widen)
      (setq esk/python--flymake-proc
            (make-process
             :name "python-flymake"
             :noquery t
             :connection-type 'pipe
             :buffer (generate-new-buffer " *python-flymake*")
             :command esk/python-flymake-command
             :sentinel
             (lambda (proc _event)
               (unwind-protect
                   (with-current-buffer (process-buffer proc)
                     (goto-char (point-min))
                     (cl-loop
                      while (search-forward-regexp esk/python-flymake-diag-regexp nil t)
                      for msg = (match-string 3)
                      for (beg . end) = (flymake-diag-region
                                         source
                                         (string-to-number (match-string 1))
                                         (and (match-string 2)
                                              (string-to-number (match-string 2))))
                      for type = (if (string-match esk/python-flymake-warn-msg-regexp msg)
                                     :warning :error)
                      collect (flymake-make-diagnostic source beg end type msg)
                      into diags
                      finally (funcall report-fn diags)))
                 (kill-buffer (process-buffer proc))))))
      (process-send-region esk/python--flymake-proc (point-min) (point-max))
      (process-send-eof esk/python--flymake-proc))))

(add-hook 'flymake-diagnostic-functions #'esk/python-flymake)
(add-hook 'python-mode-hook #'flymake-mode-on)

-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[Lele Gaifax]

Lele Gaifax <lele@metapensiero.it> writes:

> b) I had to omit the "local" flag when hooking esk/python-flymake to
>    flymake-diagnostic-functions as you did in the example: shouldn't the
>    latter be a defvar-local variable?

FTR, I figured out what I was doing wrong, and thus what João suggested was
right: the `local' argument to `add-hook' takes care of buffer-localizing the
hook variable.

ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[Lele Gaifax]

joaotavora@gmail.com (João Távora) writes:

> * Some colorful fanciness in the mode-line. Still far from flycheck's
>   (which is very good) but should be easy to get there.

I noticed a minor glitch in this: when I have two different buffers
side-by-side, both reporting some diagnostic, using the mouse-4/5 on the
single numbers in the mode-line, always moves the point in the currently
active buffer, not the one where I "click" on.

ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Flymake refactored

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

On Tue, Oct 10, 2017 at 11:40 AM, Lele Gaifax <lele@metapensiero.it> wrote:
> > * Some colorful fanciness in the mode-line. Still far from flycheck's
> >   (which is very good) but should be easy to get there.
>
> I noticed a minor glitch in this: when I have two different buffers
> side-by-side, both reporting some diagnostic, using the mouse-4/5 on the
> single numbers in the mode-line, always moves the point in the currently
> active buffer, not the one where I "click" on.

Thanks, fixed in 042b3cfbd.

[-- Attachment #2: Type: text/html, Size: 745 bytes --]