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

Use GitHub actions to run RuboCop linter #5820

Merged
merged 2 commits into from Jul 22, 2020
Merged

Conversation

gbp
Copy link
Member

@gbp gbp commented Jul 14, 2020

There seems to be ongoing issues with HoundCI, and the lately it seems
to be ignoring out RuboCop config completely.

@gbp gbp force-pushed the drop-houndci branch 2 times, most recently from 522d406 to 22d059f Compare July 14, 2020 13:32
rubocop_test.rb Outdated
# BAD: fail only signal exception
begin
fail
rescue StandardError
Copy link

Choose a reason for hiding this comment

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

Lint/SuppressedException: Do not suppress exceptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hadn't intended this to be checked - not picked up locally when running bundle exec rubocop -D rubocop_test.rb

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 wasn't raised in the GitHub action runner, I think because Hound is using Rubocop 0.80.1, instead of 0.81.0 - there was this change rubocop/rubocop#7805

Doesn't explain the other consistencies tho.

rubocop_test.rb Outdated
end
# BAD: fail only signal exception
begin
fail
Copy link

Choose a reason for hiding this comment

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

Style/SignalException: Always use raise to signal exceptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

rubocop_test.rb Outdated
# GOOD: raise only signal exception
begin
raise
rescue StandardError
Copy link

Choose a reason for hiding this comment

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

Lint/SuppressedException: Do not suppress exceptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hadn't intended this to be checked - not picked up locally when running bundle exec rubocop -D rubocop_test.rb

rubocop_test.rb Outdated
# GOOD: lines over 80 characters
_short_line = 'This line is under 80 characters.'
# BAD: lines over 80 characters
_long_line = 'This line is going to really long and be over the 80 characters.'
Copy link

Choose a reason for hiding this comment

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

Layout/LineLength: Line is too long. [81/80]

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

rubocop_test.rb Outdated
# BAD: old style hash syntax
_bad1_hash = { :bar => 'bar' }
# BAD: mixed keys syntax
_bad2_hash = { foo: 'foo', :bar => 'bar' }
Copy link

Choose a reason for hiding this comment

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

Style/HashSyntax: Use the new Ruby 1.9 hash syntax.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

rubocop_test.rb Outdated
# GOOD: new style hash syntax, with no mixed keys
_good_hash = { foo: 'foo' }
# BAD: old style hash syntax
_bad1_hash = { :bar => 'bar' }
Copy link

Choose a reason for hiding this comment

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

Style/HashSyntax: Use the new Ruby 1.9 hash syntax.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

rubocop_test.rb Outdated
#
class RubocopTest
# GOOD: trailing dots
'Hello world'.
Copy link

Choose a reason for hiding this comment

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

Layout/DotPosition: Place the . on the next line, together with the method name.

Copy link
Member Author

Choose a reason for hiding this comment

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

rubocop_test.rb Outdated
@@ -0,0 +1,35 @@
# This class is a test of Rubocop linting
Copy link

Choose a reason for hiding this comment

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

Style/FrozenStringLiteralComment: Missing frozen string literal comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gbp
Copy link
Member Author

gbp commented Jul 14, 2020

So instead of fixing HoundCI I'm purposing switching to a GitHub Action workflow. It will allow us more flexibilities with regard to gem version and RuboCop extensions. Even better it doesn't pollute the PR with comments.

@garethrees what are your thoughts? If you approve I'll drop the hound config and remove the RuboCop test file from this PR.

@gbp gbp assigned gbp and garethrees and unassigned gbp Jul 14, 2020
Copy link
Member

@garethrees garethrees left a comment

Choose a reason for hiding this comment

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

LGTM

Not a major issue, but I don't understand why we get a build step and a rubocop step. Would be clearer if we only had the rubocop check there

Screenshot 2020-07-16 at 16 31 26

gbp added 2 commits July 21, 2020 16:38
There seems to be ongoing issues with HoundCI, and the lately it seems
to be ignoring out RuboCop config completely.
@gbp gbp changed the title [WiP] Use GitHub actions to run RuboCop linter Use GitHub actions to run RuboCop linter Jul 22, 2020
@gbp gbp marked this pull request as ready for review July 22, 2020 10:15
@gbp
Copy link
Member Author

gbp commented Jul 22, 2020

Discussed on standup, we're happy for this to be merged as is.

@mysociety-pusher mysociety-pusher merged commit ac110cf into develop Jul 22, 2020
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.

None yet

3 participants