-
Notifications
You must be signed in to change notification settings - Fork 665
feat(rome_js_analyze): Implement noStaticOnlyClass
#4514
Conversation
✅ Deploy Preview for docs-rometools ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
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 your contribution :)
What about private-static fields (static #foo)?
What about TsConstructorSignatureClassMember? It seems as if we can return false here (and thus, treat it as non-static).
I think you got them right: we should treat them like other constructs.
Does this rule match on class expressions or default class exports?
Both.
The disuccsion that I had with ematipico in #4482 has certainly confused you: The rule will be in style once stabilized (in several versions from teh current one). Any new rules should be a nursery rule.
Your rule should be placed in analyzers/nursery (semantic_analyzers is reserved for rules that use the semantic model).
crates/rome_js_analyze/src/semantic_analyzers/style/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/style/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/style/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/style/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/style/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/style/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
I just noticed that the rule also matches on empty classes. I've decided to skip empty classes, since it might be the case that the user is currently typing out a new class. If this doesn't match the intentions of this rule, feel free to comment. Any Idea on how to get rid of the code duplication? I don't think it's that bad, but it doesn't seem right. |
crates/rome_js_analyze/src/analyzers/nursery/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/tests/specs/nursery/noStaticOnlyClass/invalid.ts.snap
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
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.
Thank you for you contribution!
crates/rome_js_analyze/src/analyzers/nursery/no_static_only_class.rs
Outdated
Show resolved
Hide resolved
@ematipico done and ready for squash :) |
Thanks for your contribution :) |
Summary
This is a WIP PR. Things need to be done better (for example, that match block with a lot of repeated code). I'm takign suggestions here. :)
Things to clarify:
JsBogusMember
?TsConstructorSignatureClassMember
? It seems as if we can returnfalse
here (and thus, treat it as non-static).static #foo
)? For unicorn, these seem valid: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/no-static-only-class.mdNotes:
class
keyword. This may be removed, if we decide to exclude anonymous classes from this rule.Resolves #4482
Test Plan
Some tests are there. We need to check whether this is everything needed.
Changelog
Documentation