Skip to content

Transform between braces & do/end when reflowing blocks? #120

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

Closed
xeger opened this issue Jul 22, 2022 · 2 comments · Fixed by #121
Closed

Transform between braces & do/end when reflowing blocks? #120

xeger opened this issue Jul 22, 2022 · 2 comments · Fixed by #121

Comments

@xeger
Copy link
Contributor

xeger commented Jul 22, 2022

I was rather surprised to see Syntax Tree make the following choice:

          expect(
            resulting_items.map { |i|
              { amount: i.amount }
            },
          ).to match(
            existing_model.fund.ownership_positions.current.map { |i|
              { amount: Float(i.id) }
            },
          )

It was preserving my choice of {} but when it reflowed the block to multiple lines, our convention normally would have been to switch to a do/end pair (and vice-versa).

As a workaround, I can Rubocop to auto-fix after formatting; it's a minor pain however. Is this a behavior that would be amenable to a Syntax Tree plugin? If so, perhaps I can work on contributing one to the core.

@kddnewton
Copy link
Member

So it's doing that because it's trying to be careful. Normally it would happily convert to a do..end block. However, since you're inside of a Command node, it keeps the braces.

Here's an example of what I mean:

expect foo, bar { baz }

That works fine, but if baz is actually very long:

expect foo, bar do
  bazzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
end

this actually changes the meaning to the block being given to the expect method instead of the bar method. So instead it detects that it's inside a command and keeps the braces. It does that here:

# If this is nested anywhere inside certain nodes, then we can't change
# which operators/keywords we're using for the bounds of the block.
def unchangeable_bounds?(q)
q.parents.any? do |parent|
# If we hit a statements, then we're safe to use whatever since we
# know for certain we're going to get split over multiple lines
# anyway.
break false if parent.is_a?(Statements)
[Command, CommandCall].include?(parent.class)
end
end

To fix this, as it's walking up the tree is should notice that it's inside of a method call with parentheses (like it notices it's inside a statement list) and stop walking up the tree. That would fix this.

Feel like giving this one a shot?

@xeger
Copy link
Contributor Author

xeger commented Jul 22, 2022

I'll give it a try; thanks for the advice.

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

Successfully merging a pull request may close this issue.

2 participants