Enable Rubocop in CI and automatically correct the offences we can correct #1432
Conversation
|
I'll do a manual review of the code changes before I merge, just to reduce the chance of accidental behaviour changes that aren't caught by our tests. Rubocop isn't perfect - 3e5a177 was needed to avoid broken code! |
lib/octokit/client/contents.rb
Outdated
| raise ArgumentError, "content or :file option required" if content.nil? | ||
|
|
||
| options[:content] = if Base64.respond_to?(:strict_encode64) | ||
| Base64.strict_encode64(content) |
There was a problem hiding this comment.
It looks like Rubocop has borked the indentation here 😓
There was a problem hiding this comment.
It also looks like this could be removed entirely as it's targeting Ruby 1.9.2. That means that we can merge this as is, and come back in another PR to cut out this condition.
There was a problem hiding this comment.
Issue created: #1433
| @@ -1,11 +1,9 @@ | |||
| module Octokit | |||
| class Client | |||
|
|
|||
| # Methods for the PubSubHubbub API | |||
| # | |||
| # @see https://developer.github.com/v3/repos/hooks/#pubsubhubbub | |||
| module PubSubHubbub | |||
There was a problem hiding this comment.
I somehow don't think this module is relevant anymore. We should remove it. It seems that we supported thie Pubsubhubbub protocol at some point (e.g. see here) but I think it's long gone. Although I can't find an announcement.
There was a problem hiding this comment.
Issue created: #1434
| @@ -225,10 +223,10 @@ class Unauthorized < ClientError; end | |||
| # Raised when GitHub returns a 401 HTTP status code | |||
| # and headers include "X-GitHub-OTP" | |||
| class OneTimePasswordRequired < ClientError | |||
There was a problem hiding this comment.
I don't think this code - or anything else around OTPs - is relevant with modern GitHub authentication methods.
There was a problem hiding this comment.
Issue created: #1436
| module Response | ||
|
|
||
| # Parses RSS and Atom feed responses. | ||
| class FeedParser < BaseMiddleware |
There was a problem hiding this comment.
Do we have RSS and Atom feed responses anymore? 🤔 If not, this can go.
There was a problem hiding this comment.
Issue created: #1437
octokit.gemspec
Outdated
| spec.version = Octokit::VERSION.dup | ||
| spec.metadata['rubygems_mfa_required'] = 'true' |
There was a problem hiding this comment.
We should ✂️ this, and then introduce it separately. It's a bit of a weird one to sneak in through Rubocop!
There was a problem hiding this comment.
Issue created: #1440
| if Gem::Version.new(RUBY_VERSION) < Gem::Version.new('3.2.0') | ||
| require 'pry-byebug' | ||
| end | ||
| require 'pry-byebug' if Gem::Version.new(RUBY_VERSION) < Gem::Version.new('3.2.0') |
There was a problem hiding this comment.
We should check if pry-byebug still has this problem and remove it when it doesn't
There was a problem hiding this comment.
Issue created: #1439
This change was added by a Rubocop auto-correct. It's a good thing to have, but we should add it consciously in its own PR.
|
Closing in favour of #1441 |
No description provided.