-
Notifications
You must be signed in to change notification settings - Fork 10
Remove Airbnb ruleset dependency from style guide #167
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
base: main
Are you sure you want to change the base?
Remove Airbnb ruleset dependency from style guide #167
Conversation
Signed-off-by: gokulprasanth-ni <[email protected]>
Signed-off-by: gokulprasanth-ni <[email protected]>
Signed-off-by: gokulprasanth-ni <[email protected]>
Signed-off-by: gokulprasanth-ni <[email protected]>
change/@ni-eslint-config-typescript-67da7439-6c61-46c1-9440-60866dfdaae9.json
Show resolved
Hide resolved
change/@ni-eslint-config-javascript-782dc014-2db5-4c39-94e6-ad68dd2b3470.json
Outdated
Show resolved
Hide resolved
change/@ni-eslint-config-typescript-67da7439-6c61-46c1-9440-60866dfdaae9.json
Show resolved
Hide resolved
Signed-off-by: gokulprasanth-ni <[email protected]>
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.
Just some very preliminary feedback. I still need to wrap my head around the rules changes.
change/@ni-eslint-config-typescript-67da7439-6c61-46c1-9440-60866dfdaae9.json
Show resolved
Hide resolved
Signed-off-by: gokulprasanth-ni <[email protected]>
@@ -23,7 +23,7 @@ The TypeScript extension rules (which are rules that extend the behavior of exis | |||
|
|||
### Checking the evaluated rules |
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.
In CONTRIBUTING.md we have instructions for running a diff between the original set of rules and the new set of rules after a dependency change. I ran those commands and it produced this output.
Could you evaluate what's going on with each of these? Were we depending on an older version of airbnb than the one you pulled in? Copy-paste errors? Bug with the diff tool?
It's possible that some of these will be acceptable changes but I want to make sure we're aware of their effect and we document any client-imacting changes in the change file.
> @ni/[email protected] print-evaluated-rules:diff
> node index.js --diff
import/no-import-module-exports:
old: ["error",{"exceptions":[]}]
new: ["error"]
import/consistent-type-specifier-style is new:
new: ["off","prefer-inline"]
import/no-empty-named-blocks is new:
new: ["off"]
logical-assignment-operators is new:
new: ["off","always",{"enforceForIfStatements":true}]
no-spaced-func:
old: ["error"]
new: ["off"]
spaced-comment:
old: ["error","always",{"line":{"exceptions":["-","+"],"markers":["=","!","/"]},"block":{"exceptions":["-","+"],"markers":["=","!",":","::"],"balanced":true}}]
new: ["error"]
no-constant-binary-expression is new:
new: ["off"]
no-unreachable-loop:
old: ["error",{"ignore":[]}]
new: ["error"]
no-empty-static-block is new:
new: ["off"]
no-object-constructor is new:
new: ["off"]
prefer-object-has-own is new:
new: ["off"]
prefer-regex-literals:
old: ["error",{"disallowRedundantWrapping":true}]
new: ["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.
Looking closer, anything that's "is new" with new: ["off"]
should be fine as it's no change in behavior.
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.
Most of the newly added rules are set to off
. The only notable change is no-spaced-func
. It was error
in eslint-config-airbnb-base v15.0.0
but is now off
in the newer version. If we still want to enforce that spacing style, we can update it in the upcoming PR of replacing the deprecated rules with stylistic rules as this rule is deprecated now. For spaced-comment
, our override simplifies the rule to just error
(removing Airbnb’s extra exceptions/markers); earlier it looked like the override wasn’t taking effect, but it is now working as intended.
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.
Our goal should be that this PR doesn't introduce any change in behavior so that clients can uptake it seamlessly.
Looks like no-spaced-func was deprecated and replaced with func-call-spacing a while ago (eslint 3-->4). We have a configuration for 'func-call-spacing': ['error', 'never']
so it should be fine to leave no-spaced-func
set to off
.
For spaced-comment it would be ideal to maintain the same behavior. I know I've seen some codebases that use long comments to delineate (e.g. //----
) and it would be slightly annoying if they start seeing errors that they need to change to // ----
). You mention "earlier it looked like the override wasn’t taking effect". I guess our current configuration of 'spaced-comment': 'error',
ended up falling back to the airbnb configuration with those customizations. To maintain behavior I'd prefer to keep that extra configuration in our new ruleset.
I verified this by locally adding a comment //-----
in tests/javascript/index.js
. In main that was allowed but in your branch it reported an error.
Can you also make sure prefer-regex-literals
keeps {"disallowRedundantWrapping":true}
?
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 have updated the spaced-comment
rule value as suggested. Regarding the disallowRedundantWrapping
option for the prefer-regex-literals
rule, its default value is false
, which does not match our existing configuration. Therefore, I have explicitly set this option to true
to ensure the rule continues to function as before and everything works as expected.
Signed-off-by: gokulprasanth-ni <[email protected]>
Signed-off-by: gokulprasanth-ni <[email protected]>
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.
Approved pending @jattasNI review. I don't think we need to wait for @TrevorKarjanis for this review (unless they want to chime in)
Justification
Note: Stop providing configuration for deprecated formatting rules will be taken as follow-up PR.
Implementation
Testing