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

Move us closer to the Shopify style guide #1066

Merged
merged 7 commits into from Apr 30, 2020
Merged

Conversation

DazWorrall
Copy link
Member

I needed a change of pace so decided to do some style gardening :)

Our rubocop config has drifted far from the style guide, this PR doesn't bring us all the way up to date but does knock out the quick wins. Our rubocop config is much smaller after this, and the _todo file is completely removed. There are still a few exclusions in the rubocop config I will work on in follow ups.

There only only a couple of substantive changes:

  • Switch a use of JSON.load to JSON.parse (here)
  • Check for frozen strings in OutputChunk 6fce3c0

That last one is required because write out string literals at the end of tasks, and these are now frozen:

def report_dead!
write("ERROR: Background job hasn't reported back in #{PRESENCE_CHECK_TIMEOUT} seconds.")
error!
end

Is there a cleaner fix?

@DazWorrall DazWorrall requested review from wvanbergen, casperisfine and a team April 29, 2020 15:49
@DazWorrall
Copy link
Member Author

Oh and just for fun: adopting the style guide without making any changes gives us 1163 offenses detected 😁

Copy link

@ayatsynych ayatsynych left a comment

Choose a reason for hiding this comment

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

I didn't review every file 😛but I have checked locally with the robocop 👏

@casperisfine
Copy link
Contributor

Our rubocop config has drifted far from the style guide

Well, it actually predates it significantly.

Copy link
Contributor

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

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

  • There's a few places it broke the indentations
  • Please disable Style/PerlBackrefs
  • There's other cop I'd have a lot to say about, but if it makes it easier for the occasional Shopify contributor, then fine.

@wvanbergen
Copy link
Contributor

Is there a cleaner fix?

You could also mark the literal that gets fed to this method as non-frozen: +"literal"

@DazWorrall
Copy link
Member Author

You could also mark the literal that gets fed to this method as non-frozen: +"literal"

This was a good suggestion, but I decided against it because it turns out the test suite is also peppered with write calls.

@DazWorrall
Copy link
Member Author

As for the PerlBackrefs cop I split the difference: added a match_info = foo.match pattern where that was an option, and where not reverted to $n usage and whitelisted it.

Copy link
Contributor

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

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

Thanks

@DazWorrall DazWorrall merged commit 67713cf into master Apr 30, 2020
@DazWorrall DazWorrall deleted the shopify-style-guide branch April 30, 2020 13:30
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems April 30, 2020 13:34 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

5 participants