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

Proposal: eslint --doctor #8241

Closed
alberto opened this issue Mar 13, 2017 · 16 comments
Closed

Proposal: eslint --doctor #8241

alberto opened this issue Mar 13, 2017 · 16 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion

Comments

@alberto
Copy link
Member

alberto commented Mar 13, 2017

There are a few issues related to catching problems in configuration. I think it would be useful to have an specific API and command line option to check the configuration of eslint for a given path.

It could be used:

@alberto alberto added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Mar 13, 2017
@not-an-aardvark
Copy link
Member

In my view, there are two types of configuration issues here:

  1. Problems that need to be solved right away in order to get ESLint to work (missing plugins, removed rules, invalid configs)
  2. Problems that don't need to be solved right away, but might be useful to know about (deprecation warnings)

I think the first type of issue should be addressed by throwing an error when eslint is run, because there's a high likelihood of user error. I'm not sure having a separate command would be useful for these problems, because:

  • New users might not know to use the command
  • eslint might be unable to run at all until the issue is solved.

I think the second type of issue is better suited to having a separate command (e.g. --show-deprecations), because:

  • eslint can still work correctly even before these problems are fixed
  • the problems might have been caused by upstream changes, not user error.

To summarize, I'm concerned that the --doctor flag would be trying to detect/solve too many issues at once, even though it wouldn't be the best solution for some issues.

@soda0289
Copy link
Member

soda0289 commented Mar 14, 2017

I think a command line option for checking configuration file sanity would be useful.
One use case I can give is when you have a configuration repository. For example eslint-config-custom. A build/deploy script would want to ensure this configuration file is valid, for some base eslint version, before publishing and the only current way of testing it would be to lint an empty js file, or the configuration file itself.

Having to run eslint -c eslintrc.js eslintrc.js is what I currently do on Jenkins but it feels strange and might not be appropriate in the future. Using an empty js file would seem even stranger in this case.

In my opinion having the ability to run eslint -c eslintrc.js --test-config would be a lot cleaner and more readable.

I also think that the --doctor flag is too ambiguous and seem to suggest that it would fix errors instead of just reporting them

@fregante
Copy link
Contributor

I also think that the --doctor flag is too ambiguous and seem to suggest that it would fix errors instead of just reporting them

Prior art: brew doctor only reports the issues.

@soda0289
Copy link
Member

If --doctor is a common flag I think it would be fine. Maybe call is --config-doctor to be a little more descriptive.

@alberto
Copy link
Member Author

alberto commented Mar 14, 2017

Don't get too hanged up on the name, I'm more interested in the idea. We can find a better name (--check-config?)

@not-an-aardvark It wouldn't replace reporting errors when eslint is run. About users not knowing the command, we could tell them to use it if there is a problem.

@not-an-aardvark
Copy link
Member

It wouldn't replace reporting errors when eslint is run. About users not knowing the command, we could tell them to use it if there is a problem.

Could you explain how this flag would be an improvement over just looking at the error message when eslint is run? Would it do anything other than displaying an error message?

@alberto
Copy link
Member Author

alberto commented Mar 15, 2017

It could try to detect more problems instead of a single one and it wouldn't need to run the rules. I'm not sure how feasible it would be without duplicating much logic, though, it's something we would have to explore.

It would allow us to introduce "warnings" (non fatal problems, the second category you mentioned) without worrying about them being a breaking change.

@not-an-aardvark
Copy link
Member

I'm not seeing the benefit of this. I see how it might be useful to detect multiple errors rather than a single one, but it seems like in many cases encountering a fatal problem would prevent it from doing anything else useful anyway. Validating a config without running the rules can already be done with --print-config.

As far as deprecation warnings are concerned: If they're going to be behind a flag anyway, I think it would be better to have a specific flag for them, e.g. --show-deprecations.

@soda0289
Copy link
Member

soda0289 commented Mar 15, 2017

@not-an-aardvark
Are you saying --print-config cant test configuration settings or that we should enhance it to do so. The main areas I think that need testing are invalid rule configurations, invalid rules, invalid/missing plugins, and invalid/missing parer.

Shouldn't eslint show deprecations by default? Is the only way to check if a rule is deprecated with the documentation or the change log?

@soda0289
Copy link
Member

soda0289 commented Mar 15, 2017

Also --print-config needs to take in path which is still not ideal if you just want to test one configuration file but is better than linting an empty file.

@not-an-aardvark
Copy link
Member

Also --print-config needs to take in path which is still not ideal if you just want to test one configuration file but is better than linting an empty file.

This command would still need to accept a path so that it would know which configs in subdirectories would need to be applied.


But fair enough, I see how it would be useful to have a command to check that a config is valid without actually running eslint. In that case, I suppose my concern with this proposal is that the --doctor flag seems to be doing too much at once. I think it would be better to have something like --check-config that only validates a config (using the same logic that gets used in core). I don't think deprecation warnings should be tied into the same command.

@alberto
Copy link
Member Author

alberto commented Mar 17, 2017

So you think we should have one command for fatal issues and another for informational ones?

@not-an-aardvark
Copy link
Member

Not necessarily -- I just think deprecations are separate from config validation. If there are non-fatal issues resulting from possible configuration errors (although I'm not sure what this would look like), I think the config-validation command should also report them.

@danny-andrews
Copy link
Contributor

One additional feature that would be nice would be detecting duplicate rule definitions, e.g. (I extend eslint's config which defines 'eqeqeq': 'error' and then add the same exact rule in my own config. This is definitely lower priority than warning about missing plugins/rules or deprecated rules, but might be a cool thing to add to phase two of this feature.

@danny-andrews
Copy link
Contributor

danny-andrews commented Apr 14, 2017

Also, I feel like deprecation warnings should be opt-out, not opt-in. I'm not aware of any other library/framework/tool which has deprecations as opt-in. Fussy console messages get things fixed quickly.

@alberto
Copy link
Member Author

alberto commented Jun 1, 2017

Thanks for your interest in improving eslint. Unfortunately, it looks like this issue didn't get enough support from the team and so I'm closing it. While we wish we'd be able to accommodate everyone's requests, we do need to prioritize. We've found that issues failing to reach consensus after 21 days tend to never do it, and as such, we close those issues. This doesn't mean the idea isn't interesting, just that it's not something the team can commit to.

@alberto alberto closed this as completed Jun 1, 2017
@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
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

No branches or pull requests

5 participants