Skip to content

Formatter: Add feature flag for method_signature_yield #13215

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.

Sign up for GitHub

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

Merged
straight-shoota merged 3 commits into from
Mar 25, 2023
Merged

Formatter: Add feature flag for method_signature_yield #13215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4f23c5
Formatter: Add flag `method_signature_yield` disabled by default
straight-shoota Mar 22, 2023
a3cb43e
Fix for double splat
straight-shoota Mar 23, 2023
bd46356
Remove unnecessary flag check
straight-shoota Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
212 changes: 181 additions & 31 deletions spec/compiler/formatter/formatter_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
require "spec"
require "../../../src/compiler/crystal/formatter"

private def assert_format(input, output = input, strict = false, file = __FILE__, line = __LINE__)
private def assert_format(input, output = input, strict = false, flags = nil, file = __FILE__, line = __LINE__)
it "formats #{input.inspect}", file, line do
output = "#{output}\n" unless strict
result = Crystal.format(input)
result = Crystal.format(input, flags: flags)
unless result == output
message = <<-ERROR
Expected
Expand Down Expand Up @@ -553,162 +553,149 @@ describe Crystal::Formatter do
assert_format "with foo yield bar"

context "adds `&` to yielding methods that don't have a block parameter (#8764)" do
assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo
yield
end
CRYSTAL
<<-CRYSTAL
def foo(&)
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo()
yield
end
CRYSTAL
<<-CRYSTAL
def foo(&)
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(
)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(&)
yield
end
CRYSTAL

# #13091
assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo # bar
yield
end
CRYSTAL
<<-CRYSTAL
def foo(&) # bar
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(x)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(x, &)
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(x ,)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(x, &)
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(x,
y)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(x,
y, &)
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(x,
y,)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(x,
y, &)
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(x
)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(x,
&)
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(x,
)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(x,
&)
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(
x)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(
x, &
)
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(
x, y)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(
x, y, &
)
yield
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(
x,
y)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(
x,
y, &
Expand All @@ -717,14 +704,13 @@ describe Crystal::Formatter do
end
CRYSTAL

assert_format <<-CRYSTAL,
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(
x,
)
yield
end
CRYSTAL
<<-CRYSTAL
def foo(
x,
&
Expand All @@ -733,7 +719,171 @@ describe Crystal::Formatter do
end
CRYSTAL

assert_format "macro f\n yield\n {{ yield }}\nend"
assert_format <<-CRYSTAL, <<-CRYSTAL, flags: %w[method_signature_yield]
def foo(a, **b)
yield
end
CRYSTAL
def foo(a, **b, &)
yield
end
CRYSTAL

assert_format "macro f\n yield\n {{ yield }}\nend", flags: %w[method_signature_yield]
end

context "does not add `&` without flag `method_signature_yield`" do
Copy link
Copy Markdown
Contributor

@Sija Sija Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to automate it within the assert_format method.
Checking whether the code doesn't change should be sufficient.

Copy link
Copy Markdown
Member Author

@straight-shoota straight-shoota Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking whether the code doesn't change should be sufficient.

Not it's not.
The majority of test cases actually expects the format to change. It's just two different results with and without the flag.
Putting both in the same assert would be quite tedious.

Copy link
Copy Markdown
Contributor

@Sija Sija Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was thinking of is checking it within the same call, so first it would check the formatting expectation with the given flags and next, check if the code stays the same without applying the flags.

Copy link
Copy Markdown
Member Author

@straight-shoota straight-shoota Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be too confusing. Having separate examples is fine.
This way it's easy to remove the entire non-flag group when the flag gets enabled.

assert_format <<-CRYSTAL
def foo
yield
end
CRYSTAL

assert_format <<-CRYSTAL, <<-CRYSTAL
def foo()
yield
end
CRYSTAL
def foo
yield
end
CRYSTAL

assert_format <<-CRYSTAL, <<-CRYSTAL
def foo(
)
yield
end
CRYSTAL
def foo
yield
end
CRYSTAL

# #13091
assert_format <<-CRYSTAL
def foo # bar
yield
end
CRYSTAL

assert_format <<-CRYSTAL
def foo(x)
yield
end
CRYSTAL

assert_format <<-CRYSTAL, <<-CRYSTAL
def foo(x ,)
yield
end
CRYSTAL
def foo(x)
yield
end
CRYSTAL

assert_format <<-CRYSTAL
def foo(x,
y)
yield
end
CRYSTAL

assert_format <<-CRYSTAL, <<-CRYSTAL
def foo(x,
y,)
yield
end
CRYSTAL
def foo(x,
y)
yield
end
CRYSTAL

assert_format <<-CRYSTAL, <<-CRYSTAL
def foo(x
)
yield
end
CRYSTAL
def foo(x)
yield
end
CRYSTAL

assert_format <<-CRYSTAL, <<-CRYSTAL
def foo(x,
)
yield
end
CRYSTAL
def foo(x)
yield
end
CRYSTAL

assert_format <<-CRYSTAL, <<-CRYSTAL
def foo(
x)
yield
end
CRYSTAL
def foo(
x
)
yield
end
CRYSTAL

assert_format <<-CRYSTAL, <<-CRYSTAL
def foo(
x, y)
yield
end
CRYSTAL
def foo(
x, y
)
yield
end
CRYSTAL

assert_format <<-CRYSTAL, <<-CRYSTAL
def foo(
x,
y)
yield
end
CRYSTAL
def foo(
x,
y
)
yield
end
CRYSTAL

assert_format <<-CRYSTAL, <<-CRYSTAL
def foo(
x,
)
yield
end
CRYSTAL
def foo(
x
)
yield
end
CRYSTAL

assert_format <<-CRYSTAL
def foo(a, **b)
yield
end
CRYSTAL
end

assert_format "1 + 2", "1 + 2"
Expand Down
Loading