Fix rubocop offenses [2] #401
Conversation
beengine
force-pushed
the
fix-rubocop-offences-more
branch
from
c7554bc to
8d3f146
Compare
| def self.#{name} # def self.github | ||
| @#{name} ||= Sorcery::Providers.const_get('#{name}'.to_s.camelcase).new # @github ||= Sorcery::Providers.const_get('github'.to_s.camelcase).new | ||
| end # end |
There was a problem hiding this comment.
I'm not sure what's going on here...looks like a mistake or bug? Not seeing why there should be a commented copy of the code right after the exact same code.
There was a problem hiding this comment.
no, it's Style/DocumentDynamicEvalDefinition, check Rubocop docs
| if block_given? | ||
| return false unless yield user | ||
| end | ||
| return false if block_given? && !yield(user) |
There was a problem hiding this comment.
I'm not 100% certain that this is functionally equivalent, as yield blocks tend to act strangely. Can another @Sorcery/maintainers take a look and sanity check this?
There was a problem hiding this comment.
They seem functionally equivalent to me.
yield is evaluated only after block_given? returns true, the execution timing should be the same in both cases. Do you have any concerns about that?
There was a problem hiding this comment.
@atolix is right, because of Style/SoleNestedConditional yield user -> yield(user) and this condition moved up
| if block_given? | ||
| return false unless yield user | ||
| end | ||
| return false if block_given? && !yield(user) |
There was a problem hiding this comment.
Same as above, not 100% certain that this is functionally equivalent.
| auth_hash(access_token).tap do |h| | ||
| h[:user_info] = JSON.parse(response.body).tap do |uih| | ||
| uih['email'] = primary_email(access_token) if scope =~ /user/ | ||
| uih['email'] = primary_email(access_token) if scope&.include?('user') |
There was a problem hiding this comment.
I'm not sure if this is functionally equivalent (moving from a regex match to .include? - in particular, how regex matching handles case sensitivity.
There was a problem hiding this comment.
They seem functionally equivalent in this context.
scope is expected to be either a string or nil, both =~ and .include? should behave the same way (case-sensitive partial match, falsy for nil). The only difference is that =~ returns the match index (or nil), while .include? returns a boolean.
irb(main):026> scope = 'user'
=> "user"
irb(main):027> scope&.include?('user')
=> true
irb(main):028> scope =~ /user/
=> 0
irb(main):029> scope = 'User'
=> "User"
irb(main):030> scope&.include?('user')
=> false
irb(main):031> scope =~ /user/
=> nil
irb(main):034> scope = nil
=> nil
irb(main):035> scope&.include?('user')
=> nil
irb(main):036> scope =~ /user/
=> nil| Style/OptionalBooleanParameter: | ||
| Exclude: | ||
| - 'lib/**/*' |
There was a problem hiding this comment.
It would be nice to update these at some point, but that can be a breaking change for v1.
|
|
||
| if config.prevent_non_active_users_to_login | ||
| return false, :inactive unless send(config.activation_state_attribute_name) == 'active' | ||
| if config.prevent_non_active_users_to_login && send(config.activation_state_attribute_name) != 'active' |
There was a problem hiding this comment.
To keep the functionality exactly equivalent, it would be better to use:
&& !(send(config.activation_state_attribute_name) == 'active')Instead. I'm not sure if it matters, but it would reduce the risk of some weird edge case causing a vuln.
There was a problem hiding this comment.
a == b is a complete equivalent to !a != b. Not sure if the given suggestion makes the code more readable. And rubocop will definitely ask to fix it.
| # 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__) |
There was a problem hiding this comment.
Doesn't this change the path from ../config/environment to ./config/environment? I haven't looked at how Ruby handles file pathing in a while. Another @Sorcery/maintainers should look at this.
There was a problem hiding this comment.
both give the same path
It promotes a cleaner and more direct way of constructing absolute paths within a Ruby project
Replace redundant merge! with direct hash assignment (Performance/RedundantMerge)
Replace regex match with String#include? for literal pattern (Performance/StringInclude)
beengine
force-pushed
the
fix-rubocop-offences-more
branch
from
8d3f146 to
fd88be3
Compare
|
Will add next changes in new PR |
3 participants
Please ensure your pull request includes the following: