-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Docs: Clarify the CLIEngine options #10995
Docs: Clarify the CLIEngine options #10995
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but I just left one question (inline) that I would like answered before I approve the changes.
Thanks for contributing!
docs/developer-guide/nodejs-api.md
Outdated
* `fix` - This can be a boolean or a function which will be provided each linting message and should return a boolean. True indicates that fixes should be included with the output report, and that errors and warnings should not be listed if they can be fixed. However, the files on disk will not be changed. To persist changes to disk, call [`outputFixes()`](#cliengineoutputfixes). | ||
* `globals` - An array of global variables to declare (default: empty array). Corresponds to `--global`. | ||
* `globals` - An array of global variables to declare (default: empty array). Corresponds to `--global`. Note: This differs from `.eslintrc.*` / `baseConfig`, where `globals` is an object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Do we support passing in string values that are parsed as key/value pairs (e.g., "window:true"
to signify that the window
global variable should be considered writable)? If so, that may be worth calling out here.
I think --ext
does allow the global readable/writable configuration, so I would expect it to be supported here too. Just wanted to double check.
@edmorley Friendly ping: Have you had a chance to look into the question I raised? Thanks! |
@platinumazure hi! Sorry for the delayed reply. I've tested locally and setting |
This aims to prevent some of the common pitfalls/points of confusion when configuring `CLIEngine`, such as: * it not being clear that `baseConfig` supports all of the options defined in the `.eslintrc.*` schema, and so can be used for options that are not supported as top-level `CLIEngine` arguments such as `extends` and `settings. * some of the CLIEngine options being named the same or almost the same as their `.eslintrc.*` equivalents, but being a different data type. * it not being clear that CLIEngine silently ignores invalid options (which will hopefully be fixed at some point by eslint#10272). Refs: eslint#4402 eslint#5179 eslint#6605 eslint#7247 eslint#7967 eslint#10272 webpack-contrib/eslint-loader#173 webpack-contrib/eslint-loader#252 neutrinojs/neutrino#1181
70e532f
to
1a3d550
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Thank you for the review/merge :-) |
What is the purpose of this pull request? (put an "X" next to item)
[X] Documentation update
What changes did you make? (Give an overview)
This aims to prevent some of the common pitfalls/points of confusion when configuring
CLIEngine
, such as:baseConfig
supports all of the options defined in the.eslintrc.*
schema, and so can be used for options that are not supported as top-levelCLIEngine
arguments such asextends
andsettings
..eslintrc.*
equivalents, but being a different data type.Refs:
#4402
#5179
#6605
#7247
#7967
#10272
webpack-contrib/eslint-loader#173
webpack-contrib/eslint-loader#252
neutrinojs/neutrino#1181
Is there anything you'd like reviewers to focus on?
Nothing specifically :-)