Skip to content
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

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

Closed
hediet opened this issue Jun 3, 2022 · 3 comments

Comments

@hediet
Copy link

hediet commented Jun 3, 2022

image

@lgrammel
Copy link
Contributor

lgrammel commented Jun 3, 2022

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
Copy link
Author

hediet commented Jun 3, 2022

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
Copy link
Contributor

lgrammel commented Jun 3, 2022

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

@lgrammel lgrammel closed this as completed Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants