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

Clears some rubocop offenses #281

Merged
merged 4 commits into from
Oct 26, 2018
Merged

Conversation

harman28
Copy link
Contributor

@harman28 harman28 commented Oct 6, 2018

Attempts to solve #250

Copy link
Collaborator

@nunosilva800 nunosilva800 left a comment

Choose a reason for hiding this comment

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

Thanks a lot! There are some problems though, have a look and lets see if we can make travis happy :)

@@ -45,7 +45,7 @@ def root_directory
@root_directory ||= Pathname.new(Config.root)
end

def get_binding
def binding
binding
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this is lead to an infinite loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Fixing.
I didn't perfectly understand the need for this function. Since it's private and can only be called in child classes, wouldn't it be just as effective to use binding directly in those classes instead of calling get_binding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving this in for now, I don't think I have enough of an understanding to change this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I'd say that we really don't need that method. I'm also not sure.
Give it a try - remove the method and see if the HTML reports are still properly generated.

@@ -21,7 +21,7 @@ def git(arg)
end

def self.supported?
git('branch 2>&1') && $?.success?
git('branch 2>&1') && $CHILD_STATUS.success?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is breaking the tests. We must require "English" so that this works (see rubocop/rubocop#1747).

You can add the require to the top level rubycritic module I think.

Copy link
Contributor Author

@harman28 harman28 Oct 8, 2018

Choose a reason for hiding this comment

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

Fixing this.

You can add the require to the top level rubycritic module

I was going to require only in the files that used that variable. Would requiring in the top module be better for some reason?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just because it might be used elsewhere. But I guess if this is the only file that uses $CHILD_STATUS then we can leave the require here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean: if $CHILD_STATUS is only used on RubyCritic::SourceControlSystem then it can be imported required only in this module.

@harman28
Copy link
Contributor Author

harman28 commented Oct 8, 2018

@Onumis Updated the branch. Split the changes into multiple commits for each cop fix, and also reverted some that I didn't completely understand.

Eg: Style/MethodMissing todo can be fixed with the following:

     def self.method_missing(method, *args, &block)
-      configuration.public_send(method, *args, &block)
+      if configuration.respond_to? method
+        configuration.public_send(method, *args, &block)
+      else
+        super
+      end
     end

But this causes the reek check to fail, with smell method_missing manually dispatches method call. Ideally when overriding method_missing, I believe the practice is to have a short list of methods for which we'd like special behaviour. Current code seems to want to act on all missing methods indiscriminately though.

@harman28 harman28 mentioned this pull request Oct 8, 2018
@nunosilva800
Copy link
Collaborator

@harman28 as for the method_missing issue:

I think we should first understand why we need a method_missing here.
All the config vars are defined with attr_accessor so there are method to read / write each one of them.

Probably we should just refactor how that configuration class works, but that should be a PR on it's own 😅

@nunosilva800 nunosilva800 merged commit 61bf1c7 into whitesmith:master Oct 26, 2018
@harman28 harman28 deleted the rubocop_fixes branch March 12, 2019 07:21
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.

2 participants