-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add configs #23
Add configs #23
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and it seems to work
"plugin:local-rules/error" | ||
] | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this on top because I think that it's the most common usage and it has the least amount of config
index.js
Outdated
@@ -26,6 +26,18 @@ if (!rules) { | |||
); | |||
} | |||
|
|||
function getConfig(type) { | |||
return { | |||
rules: Object.fromEntries(Object.keys(rules).map((rule) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.fromEntries is Node 12+ by the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling this out! I'll make this a major version change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a few requested changes.
index.js
Outdated
@@ -26,6 +26,19 @@ if (!rules) { | |||
); | |||
} | |||
|
|||
function getConfig(type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Update style to match
- " -> '
- add semicolons
- add trailing commas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to set up linting and CI in this repo to enforce this kind of rules 😁
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[[hejckxk'x
index.js
Outdated
module.exports = { | ||
configs: { | ||
error: getConfig("error"), | ||
warn: getConfig("warn"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this to 'all-warn'
.
That said, as an aside, severity "warn"
is typically used when introducing a new rule that will eventually be set to "error"
(source)
So I'm not sure how common it is to want everything to be warn
.
I think a useful follow-on PR would be to add support for an optional severity
field inside rules' docs
field (since plugins are allowed to put whatever they want there) that getConfig
would check first and use the value of that field when present, allowing individual rules to set themselves to warn
while the rest are still error
.
module.exports = {
'disallow-identifiers': {
meta: {
docs: {
description: 'disallow identifiers',
category: 'Possible Errors',
severity: 'warn',
},
index.js
Outdated
module.exports = { | ||
configs: { | ||
error: getConfig("error"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename error
to all
. It looks to be normal for all
to set all rules to severity error
: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/configs/all.ts
Sorry for the early review! Wasn't aware of the "ready for review" feature. |
Should be all done |
How can we move this forward? |
Sorry for the ridiculous delay. Thank you! |
Thank you for the merge! |
Closes #22
Missing: