Skip to content
This repository was archived by the owner on Jan 19, 2019. It is now read-only.

[FEAT] [BREAKING] [no-unused-vars] remove implicit dep on base rule #260

Merged
merged 2 commits into from
Dec 21, 2018

Conversation

bradzacher
Copy link
Owner

Similar to what we've done with both indent, and camelcase.
Rather than forcing users to configure both our rule and the base rule (which has been a source of confusion), this lets users just use our rule.

 {
-    "no-unused-vars": ["error", { config }],
+    "no-unused-vars": "off",
-    "typescript/no-unused-vars": "error",
+    "typescript/no-unused-vars": ["error", { config }],
 }

@j-f1
Copy link
Collaborator

j-f1 commented Dec 21, 2018

I’m wondering if we should be copying ESLint’s docs or just linking to them. I’m in favor of linking to the original docs and describing how our rule is different. This allows us to have the most up-to-date docs for the rule that we’re building on, and makes it easier for folks to see how our rule differs.

@armano2
Copy link
Contributor

armano2 commented Dec 21, 2018

It's not really good idea to do it for no unused vars, other plugins can modify eslint used property, and there can be side effects when using this with eslint-plugin-vue

@armano2
Copy link
Contributor

armano2 commented Dec 21, 2018

We shouldn't enable eslint rules anyway..

@bradzacher
Copy link
Owner Author

@armano2 - this won't break eslint-plugin-vue.
All of the eslint plugins run and interop on the same contexts.
It means that if someone is using eslint-plugin-vue/no-unused-vars, then it'll still work fine, they'll just be using our rule as their base one instead of the base eslint rule:

{
    "no-unused-vars": "off",
    "typescript/no-unused-vars": ["error", { config }],
    "vue/no-unused-vars": "error"
}

I think in this case it's fine because we are a language-level plugin.
This gives us the freedom to (if we want) switch to use parser services in future (do the parser services report on unused?), without having to do a breaking version bump.


@j-f1 I actually thought the same thing. I copied it because that's what we'd done for the other rules.
I'll raise a separate PR to rewrite all of docs together.

@armano2
Copy link
Contributor

armano2 commented Dec 21, 2018

fair enough i didn't think about

   --noUnusedLocals                    Report Errors on Unused Locals.
   --noUnusedParameters                Report Errors on Unused Parameters.

@bradzacher bradzacher merged commit 787d5e3 into master Dec 21, 2018
@bradzacher bradzacher deleted the no-unused-vars-extension branch December 21, 2018 17:42
bradzacher added a commit that referenced this pull request Dec 22, 2018
Fixes #144
Requires ~~#259~~, ~~#260~~.

- added a util to make it standardised and easier to add default config for a rule
- configured recommended based on #144
  - purposely switched `recommended` prop to be `"error" | "warning" | false`
    - inside the eslint repo, it should be `true`. otherwise it's just a property that isn't used officially by eslint. It's truthy so `eslint-docs` still work.
  - changed recommended generator to accept `"error"`/`"warning"` for more configurability.
- adjusted default config of certain rules that didn't match our recommendations.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants