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

auto load local .eslintrc #7088

Closed
wants to merge 1 commit into from

Conversation

xcodebuild
Copy link
Contributor

Load .eslintrc from project's root if existed.

@AgIt0
Copy link
Contributor

AgIt0 commented Sep 26, 2016

It should also be configurable somehow IMO, because a lot of projects I work on/I've seen keep the .eslintrc in a subfolder assets for instance

@bleggett
Copy link

bleggett commented Oct 11, 2016

This works well, and is super-needed, but I think it belongs in the general Javascript layer where eslint lives.

Also, I don't really know why Flycheck cares where the config is? Eslint will autodetect configs based on the directory structure and combine rules from ancestor .eslintrc files, so ideally we should just be able to give eslint the path of the file being checked and let it handle config discovery.

It seems weird to disable the checker because Flycheck can't find the config but the checker itself can.

This might be a Flycheck issue.

EDIT: See Using Configuration Files section of the Eslint docs: http://eslint.org/docs/user-guide/configuring

ESLint will automatically look for them in the directory of the file to be linted, and in successive parent directories all the way up to the root directory of the filesystem.

@codefalling I'd be willing to assist with getting this resolved and into master, as my day job requires eslint :)

(progn
(message rc-path)
(setq flycheck-eslintrc rc-path)))))

Copy link
Contributor

Choose a reason for hiding this comment

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

Move to the javascript layer.

@@ -51,7 +59,7 @@
(setq-local flycheck-javascript-eslint-executable eslint)))

(add-hook 'react-mode-hook #'react/use-eslint-from-node-modules)

(add-hook 'react-mode-hook #'react/use-eslintrc-from-project-root)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to the javascript layer.

@bleggett
Copy link

bleggett commented Oct 12, 2016

Looked into this more, and

  • Current versions of eslint automatically discover .eslintrc files by walking up the folder tree from the file being checked.
  • Current versions of flycheck do not try to find eslint config files, they just run eslint --print-config . which will return an exit code of 0 if eslint finds a config somewhere in the tree using the method above.
  • Because of the above, the variable flycheck-eslintrc has been deprecated in flycheck.

So really, we shouldn't ever need to explicitly tell eslint or flycheck where the .eslintrc is, as long as it is somewhere in a parent folder or the project root, which is where eslint expects to find them.

Comments from @lunaryorn in the flycheck gitter:

 In this version Flycheck doesn't try to find an eslint configuration on its own; it calls eslint --print-config for that purpose, i.e. let's eslint look for a configuration and just checks whether eslint was successful. Flycheck must verify upfront whether eslint finds a configuration because eslint errors if it doesn't find a configuration so Flycheck must not attempt to use eslint in the absence of an eslint configuration.

See also the release notes for the latest flycheck release that confim: https://github.com/flycheck/flycheck/releases/tag/30

I updated flycheck, removed some old, bad .eslintrc config files from my folder tree, verified that running eslint --print-config . reported no errors, and verified that we do not need the above fix. If you open any JS file and eslint can find a config somewhere in the current or folder or the ancestor folder tree, everything should just work, with or without projectile.

@TheBB
Copy link
Collaborator

TheBB commented Oct 12, 2016

Thank you for that @bleggett!

@robbyoconnor
Copy link
Contributor

@TheBB -- I'm pretty strong that this belongs in the javascript layer...not here...if this PR were to be accepted.

@syl20bnr
Copy link
Owner

So this PR is not needed?

If so, please merge it and then correct it in a new commit.

@robbyoconnor
Copy link
Contributor

robbyoconnor commented Oct 13, 2016 via email

@xcodebuild
Copy link
Contributor Author

As @bleggett said, this PR is not needed. Should we close this?

@robbyoconnor
Copy link
Contributor

Since it's in the wrong layer...yeah.

@TheBB
Copy link
Collaborator

TheBB commented Oct 13, 2016

Closing then since not needed, no matter what layer it's in.

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

6 participants