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 <address@hidden> 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