Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance verify-setup buffer #1478

Merged
merged 2 commits into from Apr 25, 2019
Merged

Enhance verify-setup buffer #1478

merged 2 commits into from Apr 25, 2019

Conversation

fmdkdd
Copy link
Member

@fmdkdd fmdkdd commented Jun 23, 2018

  1. Separate first checker from other checkers in verify-setup.

    This makes it clearer that only one checker can be run, even though
    multiple checkers may be available.

  2. Display next checkers in verify-setup when applicable.

    This makes it easier to debug checker chaining.

Here are some screenshots:

elisp

The checker that will effectively run is the first, and the available checkers for the buffer are below. Note that the first checker is not repeated in the available checkers. You can also see the new next checkers verification result.

Here is what it looks like with larger chains:

more-checkers

I've added two checkers foobar and foobar2 for testing. foobar was added as next-checker to c/c++-clang, and foobar2 as next-checker to foobar. This is all explicitly visible in the screenshot. The first checker will show the full chain, but the description of foobar still shows that foobar2 comes after it.

And the last example is for a buffer without any checker:

no-checkers

@cpitclaudel
Copy link
Member

cpitclaudel commented Jun 25, 2018

Thanks, the screenshot look very nice. Should we also show the other checkers in the current chain in the first section? We often run more than one checker on a single buffer, so it seems misleading to highlight just the first one.

In the past we've also discussed clarifying 'automatically disabled' vs 'may enable': maybe we could add an 'enable now' link next to 'may enable'? An in fact, if we do that, we could remove that line entirely and only show the 'enable now' link next to the thing that says 'automatically disabled'?

@fmdkdd
Copy link
Member Author

fmdkdd commented Jun 25, 2018

Should we also show the other checkers in the current chain in the first section? We often run more than one checker on a single buffer, so it seems misleading to highlight just the first one.

I tend to agree. I can put them in sequence, although it might be misleading since we cannot know if all checkers will be run (depending on the value of current-errors). At least that gets you a distinction between "these checkers may run in this buffer, and these other checkers won't definitely run until you change your config".

Plus :next-checkers can actually be multiple checkers, and only one will eventually be picked, depending on current errors . For now I'm using flycheck-get-next-checker-for-buffer which picks just one checker from :next-checkers. A quick grep tells me that we don't have any checker with multiple :next-checkers set, so this point may be moot.

In the past we've also discussed clarifying 'automatically disabled' vs 'may enable':

👍 This is confusing, I'll look into it. An enable now is a much better solution indeed.

@fmdkdd
Copy link
Member Author

fmdkdd commented Jan 18, 2019

@m-cat If you want to help, can you check out this PR and report on its usability, what could be improved, etc? I've rebased on latest master.

@m-cat
Copy link

m-cat commented Jan 19, 2019

Sure @fmdkdd, will be glad to help once I find some time 👍

Copy link
Member

@cpitclaudel cpitclaudel left a comment

Phew, finally found the time to review this. Great work as usual. I hope the comments will be helpful; I'm happy to get this merged after applying the ones you find relevant.

doc/user/syntax-checkers.rst Outdated Show resolved Hide resolved
doc/user/syntax-checkers.rst Outdated Show resolved Hide resolved
flycheck.el Outdated Show resolved Hide resolved
flycheck.el Outdated Show resolved Hide resolved
flycheck.el Outdated Show resolved Hide resolved
flycheck.el Outdated Show resolved Hide resolved
(when (and (buffer-file-name) (buffer-modified-p))
;; Save the buffer
(save-buffer))
Copy link
Member

@cpitclaudel cpitclaudel Apr 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I didn't realize we did that. Maybe we should do it through save-some-buffers instead?
I'd be happy with just a FIXME; no need to fix it now.

Copy link
Member Author

@fmdkdd fmdkdd Apr 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a FIXME. save-some-buffers would let the user choose, but then we would have a few :predicate failing due to flycheck-buffer-saved-p. Ideally we would annotate these in the verify buffer, but how do we know that flycheck-buffer-saved-p is used in the predicate? Macro magic in define-checker?

Copy link
Member

@cpitclaudel cpitclaudel Apr 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can't know; the best we could do would be to show a warning if the current buffer is unsaved.

flycheck.el Outdated Show resolved Hide resolved
flycheck.el Outdated Show resolved Hide resolved
flycheck.el Outdated Show resolved Hide resolved
@fmdkdd fmdkdd force-pushed the enhance-verify-setup branch 3 times, most recently from a50d17c to b94cd56 Compare Apr 13, 2019
@fmdkdd
Copy link
Member Author

fmdkdd commented Apr 13, 2019

@cpitclaudel I made some changes. Now we display:

  1. The first checker to run. I added back the message when there isn't any first checker.
  2. All the checkers that are part of the first checker's chain. That is a static view, not using -get-next-checker, so we disregard the current error level.
  3. All checkers that may be used in this buffer, but are not part of the current chain. I added a select button on these to easily toggle from the verify setup buffer.
  4. Any remaining (misconfigured) checker for the major mode.

Copy link
Member

@cpitclaudel cpitclaudel left a comment

LGTM now. I spotted a few small things here and there.
Thanks for the great work!

doc/user/flycheck-versus-flymake.rst Outdated Show resolved Hide resolved
flycheck.el Outdated
Return the transitive closure of the next-checker relation. The
return value is a list of checkers, not including CHECKER."
(let ((next (flycheck-get-next-checkers checker)))
(seq-uniq (append next (seq-mapcat #'flycheck-all-next-checkers next)))))
Copy link
Member

@cpitclaudel cpitclaudel Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to infloop if someone adds a checker as its own next-checker, right? I think our verification code will likely infloop as well, though, so this is not worse

Copy link
Member Author

@fmdkdd fmdkdd Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this assumes we don't have cycles. Ideally we want to ensure there are no cycles by construction, but I'm not sure how to do that in define-checker and when the :next-checkers property is altered after.

Do you want me to harden this function to ensure we don't revisit checkers twice?

Copy link
Member Author

@fmdkdd fmdkdd Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpitclaudel I've changed the code to avoid visiting checkers twice. Can you look?

flycheck.el Outdated Show resolved Hide resolved
flycheck.el Show resolved Hide resolved
@fmdkdd fmdkdd force-pushed the enhance-verify-setup branch 3 times, most recently from 4b0fda1 to 65569d3 Compare Apr 19, 2019
@cpitclaudel
Copy link
Member

cpitclaudel commented Apr 24, 2019

Thanks for the update. I just gave it a try. It looks great.
One thing that I noticed: in my Python buffer, I have flycheck-disabled-checkers set to (python-flake8 python-mypy); in the old code I get (disabled) next to these checkers, but in the new version they don't have this mark any more. Is that intended?

@fmdkdd
Copy link
Member Author

fmdkdd commented Apr 24, 2019

I get (disabled) next to these checkers, but in the new version they don't have this mark any more. Is that intended?

Maybe you are using an old version of the PR? I did have that bug, but I fixed it.

@cpitclaudel
Copy link
Member

cpitclaudel commented Apr 24, 2019

Maybe you are using an old version of the PR? I did have that bug, but I fixed it.

Yes, that seems to be it! It looks gorgeous :)
Please merge 🚀 Don't forget the changelog entry!

This makes it clearer which checkers will be run, which are merely
available, and which are misconfigured.

We also now display the potential next checkers when applicable.  This
makes it easier to debug checker chaining.

Close #1563, close #1533.
@fmdkdd fmdkdd merged commit 8997a4e into master Apr 25, 2019
3 checks passed
@fmdkdd fmdkdd deleted the enhance-verify-setup branch Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants