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

Eliminate RuboCop offenses to make CI build pass. #156

Merged
merged 1 commit into from
Aug 4, 2016

Conversation

Insti
Copy link
Contributor

@Insti Insti commented Aug 3, 2016

lib/rubycritic/configuration.rb:31:5: C: When using method_missing, define respond_to_missing? and fall back on super.

    def self.method_missing(method, *args, &block) ...

    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

lib/rubycritic/core/analysed_module.rb:30:10: C: Use methods_count.zero? instead of methods_count == 0.

      if methods_count == 0

         ^^^^^^^^^^^^^^^^^^
  1. Disabled Style/MethodMissing check for that method.
  2. Changed methods_count == 0 to methods_count.zero?

@nunosilva800
Copy link
Collaborator

LGTM! Thanks

def self.method_missing(method, *args, &block)
configuration.public_send(method, *args, &block)
end
# rubocop:enable Style/MethodMissing
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, lets rather put this in our configuration file and not in our codebase.

@troessner
Copy link
Contributor

One comment and lgtm ;)

@Insti
Copy link
Contributor Author

Insti commented Aug 4, 2016

I tried to resolve the issue by adding the required respond_to_missing? code and super call which should have fixed it, but there is a Rubocop issue: rubocop/rubocop#3358
Which means self.respond_to_missing? is not detected by Rubocop and the Rubocop error is still raised.

So I have disabled it again, but this time in the .rubocop_todo.yml file.

Once the Rubocop issue is resolved, the exclusion can be removed and everything will be good.

@Insti
Copy link
Contributor Author

Insti commented Aug 4, 2016

Adding respond_to_missing? also triggered a Reek warning that needed to be excluded.

@@ -29,7 +29,15 @@ def self.set(options = {})
end

def self.method_missing(method, *args, &block)
configuration.public_send(method, *args, &block)
if configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, why is this guard now necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a way to get the super into the method_missing to appease Rubocop.
It may just be better to disable the cop and put the code back how it was, but I wanted to at least present a version of the code that didn't raise any warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, lets rather disable the cop.

Then pls squash your PR to one commit afterwards so we cn merge this bad boy ;)

```
lib/rubycritic/configuration.rb:31:5: C: When using method_missing, define respond_to_missing? and fall back on super.

    def self.method_missing(method, *args, &block) ...

    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

lib/rubycritic/core/analysed_module.rb:30:10: C: Use methods_count.zero? instead of methods_count == 0.

      if methods_count == 0

         ^^^^^^^^^^^^^^^^^^
```

1) Add self.respond_to_missing? method to Config module.
     But disabled `Style/MethodMissing` check in `.rubocop_todo.yml`,
     since adding a `super` call does not make sense in this case.

2) Changed `methods_count == 0` to `methods_count.zero?`

3) Ignore Rake "boolean parameter" warning for respond_to_missing?
     since it implements a Ruby defined method signature:
     respond_to_missing?(symbol, include_all = false)
@Insti Insti force-pushed the Fix_rubocop_offenses branch from fb928c3 to 5c6ec18 Compare August 4, 2016 10:54
@Insti
Copy link
Contributor Author

Insti commented Aug 4, 2016

Made changes discussed in #156 (diff) and squashed.

@troessner
Copy link
Contributor

Awesome, merged, thanks!

@troessner troessner merged commit 8ec40ed into whitesmith:master Aug 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants