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

Allow config overrides #13

Merged
merged 8 commits into from
Sep 28, 2017
Merged

Allow config overrides #13

merged 8 commits into from
Sep 28, 2017

Conversation

rogeliog
Copy link
Member

This add the ability to pass a jest-runner-eslint.config.js file, with full support for https://eslint.org/docs/developer-guide/nodejs-api#cliengine

Fixes #11 and part of #10

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Ooh, nice!

README.md Outdated
```js
module.exports = {
"cliOptions": {
// ESLint cli options options here
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you include some examples, especially ones that aren't all lowercase, or have multiple words?

README.md Outdated
"cliOptions": {
// ESLint cli options options here
},
"writeOnFix": // (true|false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

couldn't this instead be fix: true in cliOptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

So my thought process was this: The ESLint Node api has a fix option but it works a bit different than --fix because it doesn't save to disk the result... In order to do that outputFixes() needs to be called.

One options that I thought is: "Always call outputFixes() when fix === true"... but I was a bit worried about it because then it would break the 1:1 mapping with the ESLint docs... If you think that is a better option I'm happy to change it 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I agree that the writeOnFix feels a awkward so I'm happy to change it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is eslint command line options, and on the CLI, --fix definitely writes to disk, I think that cliOptions is the right place to put it.

In other words, cliOptions to me maps to what eslint on the CLI supports - what the eslint API supports won't actually matter to users.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect, I agree!

README.md Outdated
```json
{
"jest-runner-eslint": {
"cliOptions": {
Copy link
Member Author

Choose a reason for hiding this comment

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

@ljharb I wonder if it is even worth having it nested under cliOptions?

I originally did it so that it is a bit more feature proof in case we add options that do no map to cliOptions, but it might be an overkill

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having been burned by this in the past, I think making cliOptions is definitely prudent, even if it's a bit annoying at times for users.

I'd much rather have an unneeded cliOptions, than need to add a new kind of option and have no place to put it :-)

README.md Outdated
}
```

See https://eslint.org/docs/developer-guide/nodejs-api#cliengine for a full
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think cliOptions should actually map to https://eslint.org/docs/user-guide/command-line-interface

Copy link
Member Author

@rogeliog rogeliog Sep 21, 2017

Choose a reason for hiding this comment

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

I think that is a good point.

Even though it would be more work, because we need to make sure that we do the correct mapping of "pseudo CLI options" to API options, but it will definitely provide a better experience.

After looking at the docs here is the list of CLI options that we could have. It does not include all of them because the API does not accept all of them. Although a lower priority, but we might need to shim the rest

Proposed option ESLint CLI option Notes
noInlineConfig --no-inline-noInlineConfig
cache --cache
cacheLocation --cache-location
config --config
envs --env Needs to be pluralized because --env can be passed multiple times
exts --ext Needs to be pluralized because --ext can be passed multiple times
fix --fix
globals --global Needs to be pluralized because --global can be passed multiple times
noIgnore --no-ingore
ignorePath --ignore-path
parser --parser
parserOptions --parser-options
plugins --plugin Needs to be pluralized because --plugin can be passed multiple times
rulesdirs --rulesdir Needs to be pluralized because --rulesdir can be passed multiple times
rules --rule Needs to be pluralized because --rule can be passed multiple times
noEslintrc --no-eslintrc
exts --ext Needs to be pluralized because --exts can be passed multiple times

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think they need to be pluralized; I think if they want more than one, it can be an array - like ext: 'foo' or ext: ['foo', 'bar']

README.md Outdated
{
"jest-runner-eslint": {
"cliOptions": {
"configFile": "/path/to/config-file"
Copy link
Collaborator

Choose a reason for hiding this comment

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

similarly, this should be config, since it should map to actual CLI options, and not to eslint's API (that very few users ever interact with)

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems good, thanks!

@rogeliog rogeliog merged commit ca1d3bc into master Sep 28, 2017
@rogeliog rogeliog deleted the add-configs branch September 28, 2017 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants