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 CLIEngine Options for Defining a Configuration are not Fully Explained in the Docs. #6605

Closed
ryanfitzer opened this issue Jul 5, 2016 · 17 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features

Comments

@ryanfitzer
Copy link
Contributor

What version of ESLint are you using?

eslint v3.0.0

What parser (default, Babel-ESLint, etc.) are you using?

default parser

Please show your full configuration:

(loaded via the CLIEngine options)

{
  'parserOptions': {
    'ecmaVersion': 6,
    'ecmaFeatures': {
        'jsx': true,
        'impliedStrict': true
    },
    env: {
      'es6': true,
      'node': true,
      'mocha': true,
      'browser': true
    },
    rules: {
      // all rules are included
    }
}

What did you do? Please include the actual source code causing the issue.

I'm trying to correctly configure the CLIEngine in a project without a root .eslintrc.* or eslintConfig property in package.json, but does contain a nested .eslintrc.* file. From what I understand from the docs, the 3 available options for my scenario are configFile, baseConfig and useSpecificConfig.

Can anyone validate that these are the expected results?

  1. baseConfig has lower precedence than project root .eslintrc.* file (including descendent .eslintrc.* files). Therefore, the object passed to baseConfig can be extended (this not addressed in Configuration Cascading and Hierarchy)?
  2. baseConfig does not satisfy the "requires configuration to run" rule.
  3. useSpecificConfig does not satisfy the "requires configuration to run" rule (the opposite is stated in Requiring Configuration to Run).
  4. useSpecificConfig only accepts an object (this option is not documented in the CLIEngine section).

What did you expect to happen?

a. I expected useSpecificConfig to satisfy the "requires configuration to run" rule.

b. I expected to ascertain the precedence of useSpecificConfig since it's not documented.

c. I expected that baseConfig would satisfy the "requires configuration to run" rule as it represents a configuration.

What actually happened? Please include the actual, raw output from ESLint.

The opposite results of what is stated in points a, b and c above.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jul 5, 2016
@nzakas
Copy link
Member

nzakas commented Jul 5, 2016

useSpecificConfig is not an option for CLIEngine, so it won't do anything if passed. The only options are configFile, which corresponds to -c, and baseConfig.

Both configFile and baseConfig should satisfy the "require configuration to run", if thats not the case, that's a bug.

@nzakas nzakas added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jul 5, 2016
@ryanfitzer
Copy link
Contributor Author

useSpecificConfig is not an option for CLIEngine, so it won't do anything if passed.

From http://eslint.org/docs/user-guide/migrating-to-3.0.0#requiring-configuration-to-run

A configuration passed to CLIEngine with the useSpecificConfig option.

Am I misunderstanding? What is useSpecificConfig an option to? An ESLint configuration? So if this exists in the config passed to baseConfig, the "require configuration to run" rule will be satisfied?

@platinumazure
Copy link
Member

Leaving a comment to note I'm tracking this. When get to a PC, I'll try to reproduce.

@ryanfitzer
Copy link
Contributor Author

Loading my configuration via an object passed to baseConfig and adding the property useSpecificConfig: true to my configuration threw Error: No ESLint configuration found.

{
  'useSpecificConfig': true,
  'parserOptions': {
    'ecmaVersion': 6,
    'ecmaFeatures': {
        'jsx': true,
        'impliedStrict': true
    },
    env: {
      'es6': true,
      'node': true,
      'mocha': true,
      'browser': true
    },
    rules: {
      // all rules are included
    }
}

@platinumazure
Copy link
Member

As @nzakas noted, useSpecificConfig is not a CLIEngine option. Unfortunately, I don't know what he meant in the migration guide. Maybe useEslintrc or configFile?

@ryanfitzer For your use case, if you want to just use a passed in configuration, you need to specify useEslintrc: false. Otherwise, ESLint will look for an eslintrc file as part of linting. Before 3.0.0, if it didn't find an eslintrc, it would use an empty config (or rather, extend your base config with an empty config, resulting in no changes). Now we throw when a user sets CLIEngine to look for an eslintrc and one is not found.

On the one hand, it's a pain to have to add a new option when invoking CLIEngine in this way. On the other hand, pre-3.0.0 your ESLint invocations were wasting cycles trying to find a config file that you probably knew didn't exist. 😄

Try adding useEslintrc: false to see if that makes the problem go away.

@ryanfitzer
Copy link
Contributor Author

Try adding useEslintrc: false to see if that makes the problem go away.

This indeed allows the baseConfig to pass the "require configuration to run" rule. But it also means that the only way one could enable their baseConfig to be extended is to provide an .eslintrc.* file in their project's root.

Is there any design reason that baseConfig was excluded as a configuration that satisfies the "require configuration to run" rule? Having the word Config in the option's name is confusing if the CLIEngine is not considered configured when passed a baseConfig.

To add to the confusion, the rules option does satisfy the requirement.

@platinumazure
Copy link
Member

No design decision- I just didn't account for baseConfig.

Can you please explain what other way you want to extend your baseConfig
besides the use of an eslintrc file, which you now can't do?
On Jul 5, 2016 8:00 PM, "Ryan Fitzer" notifications@github.com wrote:

Try adding useEslintrc: false to see if that makes the problem go away.

This indeed allows the baseConfig to pass the "require configuration to
run" rule. But it also means that the only way one could enable their
baseConfig to be extended is to provide an .eslintrc.* file in their
project's root.

Is there any design reason that baseConfig was excluded as a
configuration that satisfies the "require configuration to run" rule?
Having the word Config in the option's name is confusing if the CLIEngine
is not considered configured when passed a baseConfig.

To add to the confusion, the rules option does satisfy the requirement.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#6605 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AARWeqTU7I4vAjYiYzPB280q_ojIuyt9ks5qSv44gaJpZM4JFmSh
.

@ryanfitzer
Copy link
Contributor Author

ryanfitzer commented Jul 6, 2016

Can you please explain what other way you want to extend your baseConfig
besides the use of an eslintrc file, which you now can't do?

I actually do want to use an eslintrc file, just not one in the project root. I have eslintrc files in the project's nested folders, just not a top-level one in the root.

@ryanfitzer
Copy link
Contributor Author

Regarding:

As @nzakas noted, useSpecificConfig is not a CLIEngine option.

The condition in lib/config.js makes it clear that this is an internal property used for when a configFile is provided.

Looks like the 4th option in the Requiring Configuration to Run section is just a duplicate of the 3rd option.

@platinumazure
Copy link
Member

Okay, then I think we have at least two issues. One, baseConfig should be sufficient to avoid throwing the no config found exception. Two, the migration guide needs to be tweaked.

@ryanfitzer Am I missing anything?

@ryanfitzer
Copy link
Contributor Author

I also noticed that the rules option satisfies the requirement, but it doesn't allow for extension. Should that be expected?

@platinumazure
Copy link
Member

@ryanfitzer I think so but I'm not 100% sure on that one. It does seem redundant given the ability to use baseConfig (and useEslintrc: false if users want to avoid having an eslintrc interfere). I'm not sure what should happen there.

@ryanfitzer
Copy link
Contributor Author

ryanfitzer commented Jul 6, 2016

In the Configuration Cascading and Hierarchy section, the --rules option has the same precedence as the --configFile. Since --configFile is not extendable (useEslintrc doesn't apply to --configFile), I think rules is acting as expected/documented.

@nzakas
Copy link
Member

nzakas commented Jul 7, 2016

Sorry, clearly I was a bit overtired when I wrote the migration guide, useSpecificConfig should not be mentioned.

@platinumazure agree with the two problems you mentioned.

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 7, 2016
platinumazure added a commit to platinumazure/eslint that referenced this issue Jul 7, 2016
@platinumazure platinumazure self-assigned this Jul 7, 2016
@platinumazure
Copy link
Member

platinumazure commented Jul 7, 2016

Alright folks, I'll work on this. Here's how I'm going to do this:

  1. Create a PR to fix the currently inaccurate migration guide docs (remove useSpecificConfig).
  2. Create a PR to allow baseConfig as a configuration and add that information to the migration guide.

Let me know if there are any questions or concerns with this approach.

@nzakas
Copy link
Member

nzakas commented Jul 7, 2016

@platinumazure sounds good. Do you have an ETA?

@platinumazure
Copy link
Member

platinumazure commented Jul 7, 2016

@nzakas Now that #6618 is merged, I should be able to open the second PR today or tonight. I've already committed the changes to a branch.

EDIT: This is done, see #6625.

platinumazure added a commit to platinumazure/eslint that referenced this issue Jul 8, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

No branches or pull requests

4 participants