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

Add rubocop rake task #152

Merged
merged 2 commits into from
Jan 29, 2020
Merged

Add rubocop rake task #152

merged 2 commits into from
Jan 29, 2020

Conversation

bunnymatic
Copy link
Contributor

Problem

More dog-fooding. We want to promote the use of our carbonfive/c5-conventions
rubocop file. Let's use it here.

Solution

  • Copy the carbonfive/c5-conventions .rubocop.yml into place
  • Remove rubocop-performance because we don't really need it and
    it'd require adding another gem dev depenency - seems overkill for it's
    possible benefit on this project
  • Add lib/tasks to rakelib
  • Add rubocop.rake file to get the rubocop and rubocop:autocorrect
    tasks
  • Run rubocop:autocorrect which cleaned a few things in bin/raygun

NOTE

This repo is still not rubocop clean. There are several
not-autofixable issues that could (should?) be addressed once this is
approved.

@bunnymatic
Copy link
Contributor Author

bunnymatic commented Jan 24, 2020 •
edited
Loading

Working towards Issue #149

@bunnymatic bunnymatic mentioned this pull request Jan 24, 2020
Merged
@bunnymatic
Copy link
Contributor Author

Note: this PR is currently a branch on a branch (#147) just to keep the diff clean. PR #147 would have to come first, and it is currently waiting on #151. When these are all approved we can merge in order.

mattbrictson
mattbrictson suggested changes Jan 24, 2020
View reviewed changes
.rubocop.yml Outdated
Comment on lines 1 to 8
# Configuration hierarchy:
#
# 1. Rubocop defaults
# 2. Carbon Five defaults (this file)
# 3. Project overrides
#
# See http://rubocop.readthedocs.io/en/latest/configuration/#inheriting-configuration-from-a-remote-url for details.
#
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove this since it only relevant to the inherit_from: directive, which we are not using here.

lib/tasks/rubocop.rake Outdated
Comment on lines 1 to 3
require "rubocop/rake_task"

RuboCop::RakeTask.new
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 prefer we inline this in the Rakefile because putting this in lib implies that it is meant to be packaged and distributed as part of the raygun gem.

raygun.gemspec Outdated
@@ -23,4 +23,5 @@ Gem::Specification.new do |gem|

gem.add_development_dependency "bundler", "~> 2.0"
gem.add_development_dependency "rake", "~> 13.0"
gem.add_development_dependency "rubocop", "~> 0.79"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are not committing a Gemfile.lock I think we should pin rubocop here. New versions of rubocop often contain breaking changes, unfortunately.

Suggested change
gem.add_development_dependency "rubocop", "~> 0.79"
gem.add_development_dependency "rubocop", "0.79.0"

bin/raygun
@@ -1,10 +1,10 @@
#!/usr/bin/env ruby

File.expand_path('../../lib', __FILE__).tap do |lib|
File.expand_path("../lib", __dir__).tap do |lib|
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

.rubocop.yml

AllCops:
DisplayCopNames: true
DisplayStyleGuide: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking: It would be good to explicitly specify the version of Ruby compatibility that we are aiming for here:

Suggested change
DisplayStyleGuide: true
DisplayStyleGuide: true
TargetRubyVersion: 2.4

We can do this in a separate PR when we fix the other non-autocorrectable RuboCop issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, rubocop complains that the gemspec required version doesn't match that of the rubocop.yml so fixing this will fix a rubocop warning.

@bunnymatic bunnymatic force-pushed the chores/run-rubocop-with-c5-conventions branch from 7c0aeca to c6fe3aa Compare January 25, 2020 02:49
@bunnymatic bunnymatic force-pushed the chores/add-rubocop-rake-taks branch 2 times, most recently from 410c332 to 9b6788b Compare January 25, 2020 02:57
bunnymatic
bunnymatic commented Jan 25, 2020
View reviewed changes
Copy link
Contributor Author

@bunnymatic bunnymatic left a comment

Choose a reason for hiding this comment

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

I think I hit all the PR comments.

Look for a follow up PR which is "manually fix rubocop warnings/issues"

.rubocop.yml
Style/PreferredHashMethods:
Enabled: false

Style/SpecialGlobalVars:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Disabled this cop. It is not in the c5-conventions version yet (expect a PR soon).

It prefers (and auto corrects) the use of globals like $? with $CHILD_STATUS etc.
The problem is if you autocorrect, and try to run the code, unless you've already require 'english' in your code, this will cause the code to fail.

Before we enforce this, we should think about how to set it and also if we want to require 'english' just so we can get rubocop to do it's business. For now, I decided to leave it out.

More reading: rubocop/rubocop#1747

For the future, we could configure it to prefer or allow the perl_names which are the non-english, shortcut versions that we are already using like $? and $& etc.

Merge pull request #147 from carbonfive/chores/run-rubocop-with-c5-co…
692c8c4 
…nventions

Run autocorrections using c5-conventions/rubocop
@bunnymatic
Copy link
Contributor Author

Followup PR #153 addresses remaining non-auto-correct able issues.

Add rubocop rake task
ca91fbf 
Problem
-------

More dog-fooding.  We want to promote the use of our carbonfive/c5-conventions
rubocop file.  Let's use it here.

Solution
--------

* Copy the carbonfive/c5-conventions `.rubocop.yml` into place
* Remove `rubocop-performance` because we don't *really* need it and
it'd require adding another gem dev depenency - seems overkill for it's
possible benefit on *this* project
* Add `lib/tasks` to `rakelib`
* Add `rubocop.rake` file to get the `rubocop` and `rubocop:autocorrect`
tasks
* Run rubocop:autocorrect which cleaned a few things in bin/raygun

NOTE
----

This repo is *still* not rubocop clean.  There are several
not-autofixable issues that could (should?) be addressed once this is
approved.
@bunnymatic bunnymatic force-pushed the chores/add-rubocop-rake-taks branch from 9b6788b to ca91fbf Compare January 25, 2020 17:00
mattbrictson
mattbrictson approved these changes Jan 27, 2020
View reviewed changes
Copy link
Contributor

@mattbrictson mattbrictson left a comment

Choose a reason for hiding this comment

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

🚀

@bunnymatic bunnymatic merged commit fcf3087 into chores/run-rubocop-with-c5-conventions Jan 29, 2020
@bunnymatic bunnymatic deleted the chores/add-rubocop-rake-taks branch January 29, 2020 18:51
@mattbrictson mattbrictson mentioned this pull request Jan 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattbrictson mattbrictson mattbrictson 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.
2 participants
@bunnymatic @mattbrictson