search

p42ai / js-assistant

120+ refactorings and code-assists for Visual Studio Code
https://marketplace.visualstudio.com/items?itemName=p42ai.refactor
MIT License
119 stars 7 forks source link

Only suggest "Move initialization into constructor" when on the `=` sign, not in the entire expression. #22

Closed hediet closed 2 years ago

hediet commented 2 years ago

image

lgrammel commented 2 years ago

Thanks for reporting! I'll probably keep the activation on the left side of the expression as well (= might be too small), so you could activate the refactoring on public readonly aField =, but not on the right hand side. Would that work for you?

hediet commented 2 years ago

Yep, left side and = is also good!

Generally, you probably just shouldn't report refactorings for the parent in arbitrary nested expressions or statements.

lgrammel commented 2 years ago

I agree - the perfect activation ranges are still an open question to me. There is a tradeoff between the activation ranges not being available where you'd want/expect them, and refactorings showing up when they are clearly not intended at that position.

Currently the default is the full AST match, but that quickly gets tricky in particular for nested functions. However, for e.g. the "move intialization" refactoring, when there are constants on the right hand side (e.g. "some value"), then activating on them might be nice, but it could lead to UX inconsistencies which can be confusing.

I think it'll come down to a lot of experimentation and refinement before I can generalize, but maybe there is some general good way to do this that I just don't see yet.

For this particular case here, I ended up changing the activation range to the left hand side, the = and the first 3 characters of the right hand side, so it's readily availalbe but not showing in unwanted places.

v1.111.2 with the change should be available soon :) Thanks again