feat(rome_js_analyze): noUselessRename
#4116
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. |
| let (old_name, new_name) = match renaming { | ||
| JsRenaming::JsExportNamedFromSpecifier(x) => ( | ||
| x.source_name().ok()?.value().ok()?, | ||
| x.export_as()?.exported_name().ok()?.value().ok()?, | ||
| ), | ||
| JsRenaming::JsExportNamedSpecifier(x) => ( | ||
| x.local_name().ok()?.value_token().ok()?, | ||
| x.exported_name().ok()?.value().ok()?, | ||
| ), | ||
| JsRenaming::JsNamedImportSpecifier(x) => ( | ||
| x.name().ok()?.value().ok()?, | ||
| x.local_name() | ||
| .ok()? | ||
| .as_js_identifier_binding()? | ||
| .name_token() | ||
| .ok()?, | ||
| ), | ||
| JsRenaming::JsObjectBindingPatternProperty(x) => ( | ||
| x.member().ok()?.as_js_literal_member_name()?.value().ok()?, | ||
| x.pattern() | ||
| .ok()? | ||
| .as_any_js_binding()? | ||
| .as_js_identifier_binding()? | ||
| .name_token() | ||
| .ok()?, | ||
| ), | ||
| }; |
There was a problem hiding this comment.
Nit: this is not something specific to this rule but a pattern we could apply in general to avoid the number of ok() calls necessary in lint rules.
The idea would be to introduce a new function that returns a Result instead of an Option:
fn run(ctx: &RuleContext<Self>) -> Self::Signals {
Self::is_useless_rename(ctx).ok()
}
fn is_useless_rename(ctx) -> SyntaxResult<bool> {
// existing code but without any `ok` call
}
There was a problem hiding this comment.
The issue is that some calls return an option. e.g. x.export_as()? or .as_js_identifier_binding()?. I can use an if statement and return an early Ok(false). However I am not sure if it is worth it?
There was a problem hiding this comment.
I went down that road, and the APIs are good like this. Because some APIs return Option, trying to fit them inside a function that returns Result makes everything weird and doesn't feel right.
|
This PR is stale because it has been open 14 days with no activity. |
ematipico
left a comment
There was a problem hiding this comment.
A rebase with the newly generated files should prepare the PR for merge.
Summary
Closes #4115
This implements ESLint's rule no-useless-rename.
In contrast to ESLint, this rule does not support options and does not handle cases where identifier do not match:
Note that the formatter normalizes
\u0061toa.Computed properties are not supported (similar to ESLint):
Test Plan
Include ESLint unit tests (except unsupported cases).
Documentation