Skip to content

Commit

Permalink
Assoc uses Identity formatter if any value is nil
Browse files Browse the repository at this point in the history
The existing assoc formatter has logic to identify the case where a
value is nil (in a Ruby-3.1 style hash) and preserve the existing
formatting. For example: `{ first:, "second" => "value" }` is correctly
left as-is.

However, this logic only worked if the first assoc in the
container had the nil value - if a later assoc had a nil value, the
Identity formatter might not be chosen which could cause the formatter
to generate invalid Ruby code.

As an example, this code: `{ "first" => "value", second: }` would be
turned into `{ "first" => "value", :second => }`.

This patch pulls the nil value check up to the top of
`HashKeyFormatter.for` to ensure it takes precendence over any other
formatting selections. The fixtures have been updated to cover both
cases (nil value in first position, nil value in last position).

Fixes ruby-syntax-tree#446
  • Loading branch information
bradleybuda committed Apr 5, 2024
1 parent 8dfae19 commit 6e6feaf
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 8 deletions.
23 changes: 15 additions & 8 deletions lib/syntax_tree/node.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1553,6 +1553,9 @@ def ===(other)

def format_contents(q)
(q.parent || HashKeyFormatter::Identity.new).format_key(q, key)

# TODO: if value is +nil+ but we have chosen Rocket formatting, we need to format key as a value

return unless value

if key.comments.empty? && AssignFormatting.skip_indent?(value)
Expand Down Expand Up @@ -1784,17 +1787,21 @@ def format_key(q, key)
end

def self.for(container)
# First check for assocs where the value is nil; that means it has been
# omitted. In this case we have to match the existing formatting because
# standardizing would potentially break the code. For example:
#
# { first:, "second" => "value" }
#
container.assocs.each do |assoc|
if assoc.value.nil?
return Identity.new
end
end

container.assocs.each do |assoc|
if assoc.is_a?(AssocSplat)
# Splat nodes do not impact the formatting choice.
elsif assoc.value.nil?
# If the value is nil, then it has been omitted. In this case we have
# to match the existing formatting because standardizing would
# potentially break the code. For example:
#
# { first:, "second" => "value" }
#
return Identity.new
else
# Otherwise, we need to check the type of the key. If it's a label or
# dynamic symbol, we can use labels. If it's a symbol literal then it
Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/assoc.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,7 @@
{ "foo #{bar}": "baz" }
%
{ "foo=": "baz" }
%
{ bar => 1, baz: }
%
{ baz:, bar => 1 }

0 comments on commit 6e6feaf

Please sign in to comment.