tools icon indicating copy to clipboard operation
tools copied to clipboard

feat(rome_js_analyze): add an `inlineVariable` code action

Open leops opened this issue 3 years ago • 6 comments

Summary

This PR introduces a simple inlineVariable code assist, available on all variable declarations that only have have read-only references. The rule inlines the initializer expression for that variable into all the places it's used and removes the original variable declaration node, using logic that's largely shared with the existing noShoutyConstants rule

Test Plan

I've added a test file for the rule verifying that "read-only variables" can be inlined while writable variables are not

leops avatar Aug 02 '22 08:08 leops

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: cc4202d
Status: ✅  Deploy successful!
Preview URL: https://0bde8ee3.tools-8rn.pages.dev
Branch Preview URL: https://feature-inline-variable.tools-8rn.pages.dev

View logs

Should we limit this to only one read reference? Because if the "initializer" has side effects, we will be changing the code semantic.

xunilrj avatar Aug 02 '22 11:08 xunilrj

Should we limit this to only one read reference? Because if the "initializer" has side effects, we will be changing the code semantic.

I think for refactors we can be less strict about semantics and side-effects compared to quick fixes: they never get applied automatically, and don't have the same implied "urgency" since they're not attached to an outstanding diagnostic. The code action only gets executed in response to a very clear action intent from the user (select the variable, click on "Refactor", click on "Inline Variable") so at this point we can trust that this is really what they want to do, and they know what they're doing

leops avatar Aug 02 '22 12:08 leops

Should we limit this to only one read reference? Because if the "initializer" has side effects, we will be changing the code semantic.

I think for refactors we can be less strict about semantics and side-effects compared to quick fixes: they never get applied automatically, and don't have the same implied "urgency" since they're not attached to an outstanding diagnostic. The code action only gets executed in response to a very clear action intent from the user (select the variable, click on "Refactor", click on "Inline Variable") so at this point we can trust that this is really what they want to do, and they know what they're doing

While this is true, there's also the fact that we can't preview a refactor. It's my understanding that, as for today, these refactors are available only via LSP?

I share @xunilrj 's concerns here, and I think we should add more test cases, to make sure that we emit the correct code without breaking semantics.

ematipico avatar Aug 02 '22 12:08 ematipico

While this is true, there's also the fact that we can't preview a refactor. It's my understanding that, as for today, these refactors are available only via LSP?

Yes refactor actions can only be used through the language server integration, since they're emitted contextually for the code the user has currently selected. How these actions are presented to the user depends on the actual editor being used, but VSCode has a "Refactor Preview" panel that shows the changes about to be applied for instance:

code_action_preview

I share @xunilrj 's concerns here, and I think we should add more test cases, to make sure that we emit the correct code without breaking semantics.

I added a few more test cases (and ended up exposing a bug in the way lint rules are filtered in the code actions query)

leops avatar Aug 02 '22 15:08 leops