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

Formatter: Add feature flag for method_signature_yield #13215

Conversation

straight-shoota
Copy link
Member

Adds support for feature flags in the formatter. So far, this is only exposed in the internal API and not the CLI.
This allows us to merge formatter changes into master without enabling them in the compiler tool yet, for the purpose of bundling multiple changes into a single release as discussed in #13002.

The only feature flag so far is method_signature_yield which guards the behaviour introduced in #12951 that adds a & to yielding methods' signatures.
As a result, this behaviour is now disabled in crystal tool format per #13002 (comment)
Specs ensure proper formatter behaviour with and without the flag.

@straight-shoota straight-shoota self-assigned this Mar 23, 2023
@straight-shoota straight-shoota changed the title Formatter: Add flag method_signature_yield disabled by default Formatter: Add feature flag for method_signature_yield Mar 23, 2023
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
Contributor

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
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.

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
Contributor

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
Member Author

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.

@@ -158,6 +158,10 @@ module Crystal
@vars = [Set(String).new]
end

def flag?(flag)
Copy link
Contributor

Choose a reason for hiding this comment

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

Crystal::Program uses #has_flag?, let's use the same name here

Copy link
Member Author

Choose a reason for hiding this comment

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

The macro language uses flag?... maybe we should change has_flag? 🤔

src/compiler/crystal/tools/formatter.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 1.8.0 milestone Mar 24, 2023
@straight-shoota straight-shoota merged commit a69aa14 into crystal-lang:master Mar 25, 2023
@straight-shoota straight-shoota deleted the feature/formatter-feature-flag branch March 25, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants