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

Update: add question to confirm downgrade (fixes #8870) #8911

Merged
merged 3 commits into from Jul 18, 2017

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Jul 10, 2017

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix : fixes #8870

What changes did you make? (Give an overview)

This PR adds a question to confirm that people allow eslint --init downgrades ESLint.

(Click to open) In a `Yes` case:
  $ eslint --init
  ? How would you like to configure ESLint? Use a popular style guide
  ? Which style guide do you want to follow? Airbnb
  ? Do you use React? No
  ? What format do you want your config file to be in? JSON
  Checking peerDependencies of eslint-config-airbnb-base@latest
+ ? The style guide "airbnb" requires eslint@^3.19.0. You are currently using eslint@4.2.0.
+   Do you want to downgrade? Yes
  Installing eslint-config-airbnb-base@latest, eslint@^3.19.0, eslint-plugin-import@^2.2.0

  + eslint-config-airbnb-base@11.2.0
  + eslint-plugin-import@2.2.0
  + eslint@3.19.0
  added 27 packages, removed 779 packages, updated 3 packages and moved 61 packages in 71.853s
  Successfully created .eslintrc.json file in /path/to/repo
(Click to open) In a `No` case:
  $ eslint --init
  ? How would you like to configure ESLint? Use a popular style guide
  ? Which style guide do you want to follow? Airbnb
  ? Do you use React? No
  ? What format do you want your config file to be in? JSON
  Checking peerDependencies of eslint-config-airbnb-base@latest
+ ? The style guide "airbnb" requires eslint@^3.19.0. You are currently using eslint@4.2.0.
+   Do you want to downgrade? No
+ Note: it might not work since ESLint's version is mismatched with the airbnb config.
  Installing eslint-config-airbnb-base@latest, eslint-plugin-import@^2.2.0

  + eslint-plugin-import@2.2.0
  + eslint-config-airbnb-base@11.2.0
  updated 2 packages and moved 1 package in 11.601s
  Successfully created .eslintrc.json file in /path/to/repo

This question is shown only if peerDependencies of the chosen config is mismatched with the current local ESLint.

  • If the local ESLint does not exist, it's not shown.
  • If the local ESLint satisfies the peerDependencies, it's not shown.
(Click to open) In a not shown case:
  $ eslint --init
  ? How would you like to configure ESLint? Use a popular style guide
  ? Which style guide do you want to follow? Google
  ? What format do you want your config file to be in? JSON
  Checking peerDependencies of eslint-config-google@latest
  Installing eslint-config-google@latest

  + eslint-config-google@0.9.1
  updated 1 package in 11.153s
  Successfully created .eslintrc.json file in /path/to/repo

Is there anything you'd like reviewers to focus on?

Nothing in particular.

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@mysticatea, thanks for your PR! By analyzing the history of the files in this pull request, we identified @IanVS, @not-an-aardvark and @gyandeeps to be potential reviewers.

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly cli Relates to ESLint's command-line interface labels Jul 10, 2017
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

Nice work on this, @mysticatea! Implementation LGTM once I wrapped my head around what checkLocalESLintVersion does, so I made a suggestion for a name that might help clarify that.

* @param {Object} answers The answers object.
* @returns {boolean} `true` if the local ESLint is not found or it satisfies the required version of the chosen shareable config.
*/
function checkLocalESLintVersion(answers) {
Copy link
Member

Choose a reason for hiding this comment

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

To make the meaning of this method's boolean return value more apparent, what do you think of renaming this to hasESLintVersionConflict and inverting the return values? (This would allow you to drop the negation when calling this function on line 452.)

localESLintVersion = null;
});

it("should returns true.", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit: "should return" (no "s") here and a couple times below

@eslintbot
Copy link

LGTM

@mysticatea
Copy link
Member Author

I updated this PR as following suggestions:

  • Fixed checkLocalESLintVersionhasESLintVersionConflict
  • Fixed typo should returns

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mysticatea!

@not-an-aardvark not-an-aardvark merged commit e639358 into master Jul 18, 2017
@not-an-aardvark not-an-aardvark deleted the issue8870 branch July 18, 2017 01:22
@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 cli Relates to ESLint's command-line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eslint@4.1.1 --init installs eslint@3.19.0
6 participants