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

Fix Rubocop errors #66

Merged
merged 2 commits into from
Nov 1, 2018
Merged

Fix Rubocop errors #66

merged 2 commits into from
Nov 1, 2018

Conversation

joshcanhelp
Copy link
Contributor

Ran:

$ rubocop --auto-correct

... then manually fixed the remaining 3.

@joshcanhelp joshcanhelp added this to the 2.1.0 milestone Oct 30, 2018
@joshcanhelp
Copy link
Contributor Author

@machuga - If you have a few minutes, would you mind taking a look here? Just code style and minor changes to affect method length/complexity 🙌

@@ -19,8 +19,8 @@ group :test do
gem 'listen', '~> 3.1.5'
gem 'rack-test'
gem 'rspec', '~> 3.5'
gem 'rubocop', '>= 0.30', platforms: [
:ruby_19, :ruby_20, :ruby_21, :ruby_22
Copy link

Choose a reason for hiding this comment

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

https://www.ruby-lang.org/en/downloads/branches/ I don't see any of the current versions of Ruby (>= 2.3) available in this list. Not within scope of this PR, just something to point out

#
# Reused data
#

let(:client_id) { 'CLIENT_ID' }
let(:client_secret) { 'CLIENT_SECRET' }
let(:domain) { 'samples.auth0.com' }
let(:future_timecode) { 32503680000 }
let(:past_timecode) { 303912000 }
let(:future_timecode) { 32_503_680_000 }
Copy link

Choose a reason for hiding this comment

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

👍

@@ -234,10 +233,10 @@
def make_jwt_validator(opt_domain = domain)
options = Struct.new(:domain, :client_id, :client_secret)
Copy link

Choose a reason for hiding this comment

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

Since this is a struct/class it should be capitalized. Isn't this struct already available in this lib or is that in a different one?

Copy link

Choose a reason for hiding this comment

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

You may also want to hoist this out of this function because you're creating a new Struct type for this every invocation rather than creating an instance every time.

Options = Struct.new(...)
def make_jwt_validator(opt_domain = domain)
  OmniAuth::Auth0::JWTValidator.new(
    Options.new(opt_domain, client_id, client_secret)
  )
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mocking a struct created in OmniAuth.

Good feedback on the hoisting

@joshcanhelp
Copy link
Contributor Author

@machuga - Fixed up, passing Rubocup and tests 👍

Copy link

@machuga machuga left a comment

Choose a reason for hiding this comment

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

LGTM!

@joshcanhelp joshcanhelp merged commit 50f73e5 into master Nov 1, 2018
@joshcanhelp joshcanhelp deleted the fix-rubocop-errors branch November 1, 2018 15:20
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.

2 participants