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

Add flycheck-eslint-args var #1360

Merged
merged 1 commit into from Apr 15, 2018
Merged

Conversation

@gpittarelli
Copy link
Contributor

@gpittarelli gpittarelli commented Nov 17, 2017

It can be useful to set custom eslint arguments, for example if I want to see stricter lint rules while editing than what the project currently enforces in .eslintrc. --quiet or --maxwarnings also seem potentially useful.

Example value of flycheck-eslint-arg :

("--rule" "no-unused-vars: [2]")
@CLAassistant
Copy link

@CLAassistant CLAassistant commented Nov 17, 2017

CLA assistant check
All committers have signed the CLA.

@fmdkdd
Copy link
Member

@fmdkdd fmdkdd commented Nov 17, 2017

We tend to shy away from including catch-all arguments variables, mainly for security reasons. I believe the attack scenario here would be a directory or file local variable from foreign code setting the variable value to some malicious content, and Flycheck would eval the content automatically when you load the file.

@cpitclaudel, is this the main reason?

Here for instance, can't you use the already available --rulesdir option?

@cpitclaudel
Copy link
Member

@cpitclaudel cpitclaudel commented Nov 17, 2017

mainly for security reasons.

It's certainly one thing to be careful about, but I'm not sure how it ranks vs clean API design. Our general approach has been to favor configuration files, possibly project-local ones (because they are not Flycheck-specific), and then to fall back to specific options, one per flag (they work nicely with Emacs' customization interface, and they help maintain the invariant that tweaking Flycheck setting shouldn't be able to break a checker).

@fmdkdd
Copy link
Member

@fmdkdd fmdkdd commented Nov 17, 2017

and then to fall back to specific options, one per flag (they work nicely with Emacs' customization interface, and they help maintain the invariant that tweaking Flycheck setting shouldn't be able to break a checker).

On the other hand, it's more work for us to state out explicitly which arguments Flycheck can pass to a checker, and a more friction for users when the option they want to pass is not already in Flycheck. But I'm okay sleeping in that bed. Thanks for stating the reasoning out.

That invariant is surely worth it, and it gives me ideas about fuzzing customization variables for testing it...

Back to this PR: @gpittarelli, we would have to consider the merit of each argument (--quiet --maxwarnings and --rule) separately, and add a separte customization variable for each. Can you suggest concrete use case where these would be useful?

@cpitclaudel
Copy link
Member

@cpitclaudel cpitclaudel commented Nov 17, 2017

On the other hand, it's more work for us to state out explicitly which arguments Flycheck can pass to a checker

Definitely (hence my preference for checker-specific config files ^^)

and more friction for users when the option they want to pass is not already in Flycheck

But not too too much. For example, here, it's enough to write this:

(setf (flycheck-checker-get 'javascript-eslint 'command)
      '("eslint" "--format=checkstyle"
        (option-list "--rulesdir" flycheck-eslint-rules-directories)
        "--rule" "no-unused-vars: [2]" ;; ← this line changed
        "--stdin" "--stdin-filename" source-original))

It's definitely extra friction, but not that much :) The trickiest part is figuring out the template:

(setf (flycheck-checker-get '<checker-name> 'command)
      '(<command-line>))

I'd be OK to document this, though I'd warn against it, too.

@fmdkdd
Copy link
Member

@fmdkdd fmdkdd commented Nov 17, 2017

I'd be OK to document this, though I'd warn against it, too.

Well, the obvious downside of this kind of patching is that you have to keep that in sync whenever you update Flycheck, or you risk running into weird issues.

And, if other users have the same use case as you, then they have to use the same work around. If you submit a PR instead, then everyone can benefit ;)

Copy link
Member

@fmdkdd fmdkdd left a comment

You'll need to document that variable in doc/languages.rst. Look to other similar variables in there and do the same.

flycheck.el Outdated
@@ -8083,6 +8083,9 @@ See URL `http://www.jshint.com'."
:modes (js-mode js2-mode js3-mode rjsx-mode)
:next-checkers ((warning . javascript-jscs)))

(flycheck-def-args-var flycheck-eslint-args javascript-eslint
:package-version '(flycheck . "0.32"))

This comment has been minimized.

@fmdkdd

fmdkdd Feb 10, 2018
Member

The version string should be "32" not "0.32".

@fmdkdd
Copy link
Member

@fmdkdd fmdkdd commented Feb 10, 2018

So I think I would be in favor for the new variable after all. Mostly because of the precedents in other checkers.

We already have configuration files for eslint, but this allows one to override it locally.

@cpitclaudel Are you fine with this?

@gpittarelli Sorry for the delay. If you make a couple of changes, I think we can merge this PR.

@cpitclaudel
Copy link
Member

@cpitclaudel cpitclaudel commented Feb 10, 2018

@fmdkdd Yup, I'm OK with it :) Thanks for the discussion!

@fmdkdd
fmdkdd approved these changes Feb 11, 2018
@fmdkdd
Copy link
Member

@fmdkdd fmdkdd commented Feb 11, 2018

@gpittarelli Great! Thank you. One last thing: Please squash your commit before we can merge.

@gpittarelli gpittarelli force-pushed the gpittarelli:eslint-args-var branch from 93f7890 to 5575483 Feb 12, 2018
@viniciussbs
Copy link

@viniciussbs viniciussbs commented Apr 15, 2018

Hi, guys! Any update on this? With this var I can pass --ext .js --ext .graphql to also lint GraphQL files.

@cpitclaudel cpitclaudel merged commit 479ffaf into flycheck:master Apr 15, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@cpitclaudel
Copy link
Member

@cpitclaudel cpitclaudel commented Apr 15, 2018

Woops. Thanks for the reminder!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants