-
Notifications
You must be signed in to change notification settings - Fork 173
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
Code actions for attribute accessor creation #2739
base: main
Are you sure you want to change the base?
Code actions for attribute accessor creation #2739
Conversation
I'm unsure whether the calculation of target_range is appropriate. |
code_actions << Interface::CodeAction.new( | ||
title: CREATE_ATTRIBUTE_READER, | ||
kind: Constant::CodeActionKind::EMPTY, | ||
data: { range: @range, uri: @uri.to_s }, | ||
) | ||
code_actions << Interface::CodeAction.new( | ||
title: CREATE_ATTRIBUTE_WRITER, | ||
kind: Constant::CodeActionKind::EMPTY, | ||
data: { range: @range, uri: @uri.to_s }, | ||
) | ||
code_actions << Interface::CodeAction.new( | ||
title: CREATE_ATTRIBUTE_ACCESSOR, | ||
kind: Constant::CodeActionKind::EMPTY, | ||
data: { range: @range, uri: @uri.to_s }, | ||
) |
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.
Currently, we've been showing all refactoring code actions unconditionally because we only hand 2 or 3.
As this number grows, we are going to need to start being more selective about when to show things to the user.
What do you think about using the range with Document#locate_first_within_range
and then we only make these refactors available if we found an instance variable inside?
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.
Sounds good; we can proceed with that. For now, Document#locate_first_within_range
should be sufficient for handling instance variables.
However, if we later need to display a toggle block-style action based on the presence of a block in the selected region, we’ll need to call Document#locate_first_within_range
again to locate the call node.
If we create a method that accepts a list of desired nodes and returns only those within a specified range (similar to Document#locate_first_within_range but designed to find all specified options rather than just one). we could then use this list to determine which code actions to provide.
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.
@vinistock Which approach should we take to move forward for this?
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.
Sorry, just getting back to this now. Let's use locate_first_within_range
to only show the code actions if we're on an instance variable.
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.
Let's consider this user behavior: we have an InstanceVariableWriteNode like @foo = 1, and the user's selection includes only @foo. Using locate_first_within_range
would return nil since the selection range does not cover the entire node, preventing us from providing attribute-related actions. The same applies to all types of write nodes.
Is there an alternative approach or implementation in our library to detect such cases?
Motivation
Closes #2503
Implementation
Selecting the first valid instance variabe from the user's selection and generating an attribute accessor method just below the start of the corresponding class/module definition.
Automated Tests
Added
Manual Tests
Select a line which contains instance variables, click the light bulb and select desired attribute accessor.
Screencast.from.20-10-24.06.18.44.PM.IST.webm