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

using the local eslint and eslint config first #13

Merged
merged 4 commits into from
Jul 15, 2016
Merged

using the local eslint and eslint config first #13

merged 4 commits into from
Jul 15, 2016

Conversation

xwartz
Copy link
Contributor

@xwartz xwartz commented Jul 4, 2016

using the local eslint and eslint config first, if not available, then using the settings config


@staticmethod
def get_eslint_config(root, folders):
cfs = ['.eslintrc.js', '.eslintrc.yaml', '.eslintrc.yml', '.eslintrc.json', '.eslintrc']
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we should be worrying about the configuration file. We call eslint from the command line passing the file path to be fixed. We pass a config file only if a project sets a specific one. Otherwise eslint calculates the configuration of that file based on the .eslintrc files up the tree to the root from that file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any plugins or shareable configs that you use must also be installed globally to work with a globally-installed ESLint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

I don't believe regular configuration files like .eslintrc are considered shareable configs in eslint terminology.

From: http://eslint.org/docs/developer-guide/shareable-configs

Shareable configs are simply npm packages that export a configuration object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the plugins have to be installed globally.

@TheSavior
Copy link
Owner

TheSavior commented Jul 5, 2016

Does anyone from @eslint have additional thoughts on the ideal behavior of this plugin and the interaction with the eslint cli?

@platinumazure
Copy link

platinumazure commented Jul 6, 2016

I'm not 100% sure what's going on and I'm definitely not the best resource for this, but here's what I can try to offer:

  1. Whether you run ESLint globally or locally, it will eventually be passed a full path to a JS file for linting. (The path might be relative when you invoke the executable, but it will eventually be resolved to an absolute path.) We use that path to search for configuration files, going up the directory and using some fallbacks (see documentation). So basically, there's no need to look for config files locally in the project even if you're running ESLint globally.
  2. Supposing a local configuration file (in the same directory as the file for linting, or an ancestor file, or in user's home directory) extends a shareable config. (Note that a shareable config means an npm package exporting an ESLint configuration-- extending a local file or eslint:recommended doesn't count.) Only in that case, the shareable config needs to be globally installed (if running ESLint globally) or locally (if running ESLint locally through dev dependencies). ESLint resolves those packages internally, so there's no need to hunt that down in the plugin integration code.
  3. So all that really needs to be done, if you want to run a "local" ESLint if available, is to do this logic:
    1. For the file to be linted, look in its containing directory and recurse upward until you find a package.json file. (If you don't find one, skip the next step.)
    2. From the package.json directory, add ./node_modules/.bin to the front of the user's PATH.
    3. Run eslint using system() or whatever other method.

You might find this article helpful (it's in Node but the concepts should be fairly easily translatable to Python).

@TheSavior
Copy link
Owner

@xwartz Do you feel like you have a clear path forward on this PR based on @platinumazure's response?

My thoughts are that it would be great to have this PR reduced to only try to find the local eslint if it exists. Following @platinumazure's step 3.

@xwartz
Copy link
Contributor Author

xwartz commented Jul 11, 2016

Hi, @TheSavior
I updated this PR following @platinumazure's logic.

# find package.json first
if os.path.isfile(pkg):
# get eslint from local node_modules
return os.path.join(root, esl)
Copy link
Owner

Choose a reason for hiding this comment

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

I believe that if you have a package.json, but do not have eslint installed locally, this would return the path as if you did have it installed, which then wouldn't be caught by the None check on 172.

I think there needs to be a check here to ensure that os.path.join(root, esl) is a file

Copy link
Owner

Choose a reason for hiding this comment

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

findup is a pretty common pattern / function, looking at how it is typically implemented might help make sure we don't miss an edge case here.

@xwartz
Copy link
Contributor Author

xwartz commented Jul 11, 2016

@TheSavior
Thanks for your code review and advise. I'm new to python.
I have read the findup source code. And copied the code.
Should i import it as a module?
Thanks.

@TheSavior
Copy link
Owner

@xwartz This looks great.

I'm actually pretty new to python myself, most of the stuff I do these days in javascript.

Thanks for the PR.

@TheSavior TheSavior merged commit b3abb02 into TheSavior:master Jul 15, 2016
@TheSavior
Copy link
Owner

Released as 2.1.0

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