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.

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

Jump to bottom

Updates Rubocop Gems #1254

Merged
merged 1 commit into from
Aug 9, 2020
Merged

Updates Rubocop Gems #1254

merged 1 commit into from
Aug 9, 2020

Conversation

tlynam
Copy link
Contributor

@tlynam tlynam commented Jul 27, 2020 •
edited
Loading

  • Sets rubocop-performance minor version to prevent unexpected offenses from appearing in CI.
  • Updates rubocop gems.

Thank you for your contribution!

Check the following boxes:

  • Wrote good commit messages.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

@tlynam tlynam requested a review from aried3r July 27, 2020 07:28
@tlynam tlynam marked this pull request as ready for review July 27, 2020 07:28
@tlynam
Copy link
Contributor Author

tlynam commented Jul 27, 2020

Hi @aried3r, I created a PR to set the RuboCop Performance version so we stop introducing unexpected offenses in CI. What do you think?

@aried3r
Copy link
Member

aried3r commented Jul 27, 2020

I like it! It's there a reason you used 1.6.1 instead of 1.7.1?

@aried3r
Copy link
Member

aried3r commented Jul 27, 2020

Maybe we should to the same for rubocop-rspec as well.

@tlynam tlynam force-pushed the set-rubocop-performance-minor-version branch 2 times, most recently from 53e8930 to abc82a6 Compare August 3, 2020 03:01
tlynam
tlynam commented Aug 3, 2020
View reviewed changes
lib/paper_trail/events/base.rb Outdated
@@ -218,7 +218,7 @@ def notable_changes
end

# @api private
def notably_changed
def notably_changed # rubocop:disable Metrics/CyclomaticComplexity
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rubocop reported this method but it appears too complex to refactor for this PR. What do you think of disabling for this method?

Copy link
Member

Choose a reason for hiding this comment

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

I assume this is because of recent changes in rubocop, see here: rubocop/rubocop#8149.

They also bumped the default from 6 to 7.

paper_trail does set a lower goal. Do you by any chance remember how high complexity was? But I agree this is too much for this PR. But rather than disabling it, I'd increase it in .rubocop_todo.yml.

paper_trail/.rubocop_todo.yml

Lines 12 to 13 in fa8b1b2

Metrics/CyclomaticComplexity:
Max: 7 # Goal: 6

I'm a bit hesitant to change it, since @jaredbeck has set these rules and goals. WDYT, @jaredbeck?

tlynam
tlynam commented Aug 3, 2020
View reviewed changes
spec/dummy_app/config.ru
@@ -2,5 +2,5 @@

# This file is used by Rack-based servers to start the application.

require ::File.expand_path("../config/environment", __FILE__)
require ::File.expand_path("config/environment", __dir__)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rubocop reported this. I'm not familiar with this but I'm assuming it's a safe change?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's alright.

tlynam
tlynam commented Aug 3, 2020
View reviewed changes
spec/models/version_spec.rb
@@ -12,7 +12,7 @@ module PaperTrail
specify { expect(PaperTrail.serializer).to be PaperTrail::Serializers::YAML }

it "store out as a plain hash" do
expect(value =~ /HashWithIndifferentAccess/).to be_nil
expect(value).not_to include("HashWithIndifferentAccess")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rubocop reported and it's easy to fix

@tlynam
Copy link
Contributor Author

tlynam commented Aug 3, 2020

Hi @aried3r, that sounds good. Initially was trying to making the smallest change that resolved it but how about we update all the rubocop gems? What do you think?

@tlynam tlynam changed the title Sets rubocop-performance minor version to prevent unexpected offenses from appearing in CI Updates Rubocop Gems Aug 3, 2020
@tlynam tlynam mentioned this pull request Aug 3, 2020
Merged
6 tasks
@jaredbeck
Copy link
Member

I .. set the [development gem] version so we stop introducing unexpected offenses in CI. What do you think?

I'm 100% in favor of locking down development gems to make CI more reliable. Nice work!

[RuboCop] bumped the default [CyclomaticComplexity] from 6 to 7.

Since we haven't achieved 6 yet, this is convenient for us 😆 . Seriously, I don't see a big difference between 6 and 7 and I'm happy either way, thanks!

@aried3r
Copy link
Member

aried3r commented Aug 6, 2020

Alright, then I think we should set the internal goal to 7 (RuboCop's new default) and the Max value in .rubocop_todo.yml to whatever is the current maximum according to rubocop instead of selectively disabling individual blocks of code.

Since by now rubocop 0.89.0 has been released, I'd like to get this shipped soon. My apologies for the slow replies.

@tlynam, feel free to leave it at 0.88.0 or update to 0.89.0 if you see fit. Thanks!

@tlynam tlynam force-pushed the set-rubocop-performance-minor-version branch from abc82a6 to 6f83983 Compare August 8, 2020 20:18
@tlynam
Updates rubocop gems
1987422 
- Manually specifies minor version so it doesn't automatically update minor verison during CI tests.
- Updates other rubocop gems.
- Resolves cops
@tlynam tlynam force-pushed the set-rubocop-performance-minor-version branch from 6f83983 to 1987422 Compare August 8, 2020 20:25
@tlynam
Copy link
Contributor Author

tlynam commented Aug 8, 2020

Hi @aried3r, what do you think of keeping it at 0.88.0 for this PR because 0.89.0 introduces another 7 rubocop offenses?

aried3r
aried3r approved these changes Aug 9, 2020
View reviewed changes
Copy link
Member

@aried3r aried3r left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! And yes, lets skip 0.89.0 for this PR.

@aried3r aried3r merged commit 8f73c15 into paper-trail-gem:master Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aried3r aried3r aried3r approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tlynam @aried3r @jaredbeck