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

New: add plugin:node/recommended config #39

Merged
merged 2 commits into from
Jun 17, 2016

Conversation

rkaneko
Copy link
Contributor

@rkaneko rkaneko commented Jun 14, 2016

This pull request is one of PoC for #38 .

If this is merged, we can configure .eslintrc using plugin:node/recommended.

like this

{
  "extends": [
    "plugin:node/recommended"
  ],
  "parserOptions": {
    "ecmaVersion": 6
  },
  "plugins": [
    "node"
  ]
}

This configure equals to

{
  "parserOptions": {
    "ecmaVersion": 6
  },
  "plugins": [
    "node"
  ],
  "env": {
    "node": true,
    "es6": true
  },
  "rules": {
     "node/no-deprecated-api": "error",
     "node/no-missing-import": "error",
     "node/no-missing-require": "error",
     "node/no-unpublished-import": "error",
     "node/no-unpublished-require": "error",
     "node/no-unsupported-features": ["error",{"version": 4}],
     "node/shebang": "error"
  }
}

@rkaneko rkaneko force-pushed the plugin-node-recommended-configs branch from 5b49c52 to 4827e1c Compare June 14, 2016 13:42
@mysticatea
Copy link
Owner

Thank you for the PR and issue!

It's good that we have the recommended setting.
I confirmed that the code works fine. 👍
Great.

But I have 3 concerns on this implement.

  • This setting has "node/no-unsupported-features": ["error",{"version": 4}].
    But I prefer "node/no-unsupported-features": "error" instead.
    If version option was omitted, the rule sees "engines" field of package.json. I'd like to recommend a use of the "engines" field.

  • By ESLint's semantic versioning policy, a change of a preset setting needs major bump (in fact, it was the main reason why I have not made recommended setting). So I'd like to make it as people can check the recommended setting (and its history) easily.
    What about conf/recommended.json file?

    {
         "env": {
              "es6": true,
              "node": true
         },
         "rules": {
              "node/no-deprecated-api": "error",
              "node/no-missing-import": "error",
              "node/no-missing-require": "error",
              "node/no-unpublished-import": "error",
              "node/no-unpublished-require": "error",
              "node/no-unsupported-features": "error",
              "node/shebang": "error"
          }
    }

    Then index.js is:

    ...
    },
    configs: {
        recommended: require("./conf/recommended.json")
    }
    ...
    

@rkaneko
Copy link
Contributor Author

rkaneko commented Jun 15, 2016

@mysticatea

Thank u so much for your reviewing.

I could not understand well eslint-plugin-node's design of rule node/no-unsupported-features and ESLint's semantic versioning policy.

I took your comments into consideration.

  • changed node/no-unsupported-features's recommended rule from ["error": {"version": 4}] to "error".
  • extracted recommended config into conf/recommended.json.

And, to realize above things I edited some codes.

  • extracted some helper functions from scripts/generate-index.js.
  • edit npm run builld scripts.

If u have some better opinions for my commits, pls let me know.

Thanks.

@mysticatea
Copy link
Owner

mysticatea commented Jun 16, 2016

Ah, I'm sorry for my explanation is not enough.
I'd like to stop auto-generation of recommended.json.
Because a change of recommended.json needs a major release, so when I will change it, I'd like to modify it expressly.

@rkaneko rkaneko force-pushed the plugin-node-recommended-configs branch from fd311c3 to 662f107 Compare June 16, 2016 14:12
@rkaneko rkaneko force-pushed the plugin-node-recommended-configs branch from 662f107 to c21eed5 Compare June 16, 2016 14:22
@rkaneko
Copy link
Contributor Author

rkaneko commented Jun 16, 2016

I'm sorry I couldn't understand your intension.

I reverted and created conf/recommended.json and fixed scripts/generate-index.js.

If you have some advices, please let me know.

Thanks!

@mysticatea
Copy link
Owner

Thank you so much!
I will merge this after I get back to my PC.

@mysticatea mysticatea merged commit 22e972d into mysticatea:master Jun 17, 2016
@rkaneko
Copy link
Contributor Author

rkaneko commented Jun 17, 2016

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants