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

Set Style/TrailingCommaInArguments's EnforcedStyleForMultiline to comma #444

Merged
merged 1 commit into from Sep 13, 2022

Conversation

rafaelfranca
Copy link
Member

Differently from the Hash and Array versions of this rule, this one uses the existence of parentheses to determine if a comma is needed or not, not if the method is multiline. As our style guide require parentheses for all method calls, so this rule can't use the consistent_comma option.

Differently from the Hash and Array versions of this rule, this one
uses the existence of parentheses to determine if a comma is needed
or not, not if the method is multiline. As our style guide require
parentheses for all method calls, so this rule can't use the
consistent_comma option.
@rafaelfranca rafaelfranca requested a review from a team as a code owner September 13, 2022 19:45
Copy link

@d1egoaz d1egoaz left a comment

Choose a reason for hiding this comment

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

🚀 just a small comment

@@ -17,7 +17,7 @@ def test_config_is_unchanged
Rake::Task["config:dump"].invoke(tempfile.path)

diff = Diffy::Diff.new(
original_config, tempfile.path, source: "files", context: 5,
original_config, tempfile.path, source: "files", context: 5
).to_s
Copy link

Choose a reason for hiding this comment

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

What about also adding a test for where the ) is in the same line as the last item? that would have helped caching the issue with #432

      diff = Diffy::Diff.new(
        original_config, tempfile.path, 
        source: "files", context: 5).to_s

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a test for an unrelated thing that needed to change because of this rule, but we don't have tests for each rules in this repository. I usually do that with Shopify/shopify.

Copy link
Contributor

@sambostock sambostock left a comment

Choose a reason for hiding this comment

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

comma is what we used to have it set to, isn't it?

@rafaelfranca
Copy link
Member Author

We had it disabled. I think comma is the one that makes most sense if we are to enable it.

@rafaelfranca rafaelfranca merged commit 56392c4 into main Sep 13, 2022
@rafaelfranca rafaelfranca deleted the rm-comma branch September 13, 2022 21:02
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems September 13, 2022 21:26 Inactive
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 this pull request may close these issues.

None yet

3 participants