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

Disable syntax checkers which are not installed #1116

Merged
merged 1 commit into from Sep 28, 2016

Conversation

Projects
None yet
3 participants
@lunaryorn
Contributor

lunaryorn commented Sep 28, 2016

Following up to #1098 I figured that it'd be a good idea to disable syntax checkers that are not installed upfront through the new :enabled feature, because it's unlikely that this changes in every syntax check, so it really doesn't need to be in :predicate.

Additionally, :enabled takes no parameters anymore and now runs with the checker's default directory, both changes for consistency with :predicate.

As a side-effect this should fix #1115 because if missing eslint is now disabled before its own :enabled even runs.

@lunaryorn lunaryorn force-pushed the disable-missing-syntax-checkers branch 2 times, most recently from 27f5c9a to 581f79c Sep 28, 2016

Disable syntax checkers which are not installed
Follow up to GH-1098, and hopefully fixes GH-1115

@lunaryorn lunaryorn force-pushed the disable-missing-syntax-checkers branch from 581f79c to bd33d6e Sep 28, 2016

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 28, 2016

I did cursory testing, and things seem to work fine, but I'd be happy if someone else could also take a look 😊

@Simplify What do you think?

(let* ((default-directory (flycheck-compute-working-directory checker))
(executable (flycheck-find-checker-executable checker))
(lambda ()
(let* ((executable (flycheck-find-checker-executable 'javascript-eslint))

This comment has been minimized.

@cpitclaudel

cpitclaudel Sep 28, 2016

Member

Did you really mean to remove the default-directory here? Will the call to executable return the same thing?

This comment has been minimized.

@cpitclaudel

cpitclaudel Sep 28, 2016

Member

Ugh, sorry. Makes sense now.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Sep 28, 2016

LGTM :) I think this is a nice change. I was a bit reluctant about the scope creep of :enabled at first, but this does seem like the right approach.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Sep 28, 2016

Additionally, :enabled takes no parameters anymore

Neat :)

@Simplify

This comment has been minimized.

Member

Simplify commented Sep 28, 2016

@lunaryorn it looks great. I was trying to think about situation when you don't want to change default-directory to the location of file in the buffer. Only thing that I can think of is new elixir checker that we got few weeks ago, that requires "project" directory to call mix credo or mix dogma (can't remember witch one is now in flycheck). On the other hand it does not uses :enabled yet and if ever required default-directory can be set again inside :enabled.
I don't have time right now to test of this works with installed eslint, but will have time tomorrow. This solves current crashes when eslint is not installed, so you don't need to wait for me, it's better than current melpa unstable package :) LGTM :)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 28, 2016

@Simplify Thanks for your comment, LGTM for me too then.

I guess we should refactor the Elixir syntax checker at some point to use :enabled, since it makes no sense to check for the project definition every single time, as it's not going to change that frequently. But that's for another time, /cc @flycheck/elixir

@lunaryorn lunaryorn merged commit cea7e2f into master Sep 28, 2016

4 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@lunaryorn lunaryorn deleted the disable-missing-syntax-checkers branch Sep 28, 2016

@Simplify

This comment has been minimized.

Member

Simplify commented Sep 29, 2016

@lunaryorn I just tested this with current Melpa unstable package with several JS projects, with or without ESLint configuration. It works. Not only that, it's great that I don't need to disable eslint checker in projects .dir-locals.el anymore for projects where I don't use ESLint, for example Ember projects that have JSHint integration ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment