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

Add inline variable refactor #2504

Open
vinistock opened this issue Aug 28, 2024 · 2 comments
Open

Add inline variable refactor #2504

vinistock opened this issue Aug 28, 2024 · 2 comments
Labels
enhancement New feature or request help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale server This pull request should be included in the server gem's release notes

Comments

@vinistock
Copy link
Member

Add a new code action to inline a local variable. For example:

# BEFORE
def foo
  a = 5 * 10
  puts a
  b = a - 5
end

# AFTER
def foo
  puts 5 * 10
  b = 5 * 10 - 5
end

Here's an example PR adding a refactor end to end #2372.

@vinistock vinistock added enhancement New feature or request help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale server This pull request should be included in the server gem's release notes labels Aug 28, 2024
@ttilberg
Copy link

I was reading through this issue, and wanted to note that the resulting expression may need to have parenthesis in order to keep order of operations good. I'm not sure if it would be possible to calculate that it's not needed. Some examples counter to the example in the issue:

# Subtraction before multiplication
# BEFORE
def foo
  a = 5 - 10
  puts a  # => -5
  b = a * 5  # => -25
end

# Without ()
def foo
  puts 5 - 10 # => -5
  b = 5 - 10 * 5 # => **-45**
end

# With ()
def foo
  puts (5 - 10) # => -5
  b = (5 - 10) * 5 # => **-25**
end

I can't think of an example for this other thought quick enough, but I suspect there would be issues with certain method calling instances as well, akin to what you see with 1..10.map vs (1..10).map... you know, all those classic cases where you end up needing parens afterall.

I just wanted to highlight this concern as I feel like this refactoring tool could create some easy-to-miss bugs if it doesn't wrap the expression in parens.

@ttilberg
Copy link

Maybe there should be a check for "does the replacement candidate, or replacement targets include any method calls" in the ast? And if so, we should use parens? This may be naïve, but it seems like that's where things can get sticky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale server This pull request should be included in the server gem's release notes
Projects
None yet
Development

No branches or pull requests

2 participants