Fix up approx 20% of the rubocop issues in aruba #654
Conversation
| include Environment | ||
| include Filesystem | ||
| include Text | ||
| include Bundler |
| @@ -20,5 +20,9 @@ def method_missing(name, *args) | |||
|
|
|||
| config[name] | |||
| end | |||
|
|
|||
| def respond_to_missing?(*) | |||
| true | |||
There was a problem hiding this comment.
This should probably do something like config.key? name.
lib/aruba/api/core.rb
Outdated
| @@ -15,7 +15,7 @@ module Core | |||
|
|
|||
| # Aruba Runtime | |||
| def aruba | |||
| @_aruba_runtime ||= Runtime.new | |||
| @aruba ||= Runtime.new | |||
There was a problem hiding this comment.
This may break things since Aruba::Api::Core ends up being included elsewhere.
There was a problem hiding this comment.
I checked everywhere that module was included and there was no mention of @aruba anywhere in the entire codebase where that module was included
There was a problem hiding this comment.
I think this change may be responsible for a lot of the build failures.
| @@ -22,6 +22,10 @@ def nil? | |||
| def method_missing(*) | |||
| fail NoCommandHasBeenStoppedError, 'No last command stopped available' | |||
| end | |||
|
|
|||
| def respond_to_missing?(*) | |||
There was a problem hiding this comment.
I suppose this is correct for now, but I'd really like to simplify this design.
lib/aruba/platform.rb
Outdated
| @@ -4,18 +4,10 @@ | |||
| # Aruba | |||
| module Aruba | |||
| PLATFORM_MUTEX = Mutex.new | |||
| end | |||
| PLATFORM = [Platforms::WindowsPlatform, Platforms::UnixPlatform].find(&:match?) | |||
There was a problem hiding this comment.
Which cop makes you change Platform to PLATFORM? I feel this file should at least define Aruba::Platform. Renaming may even be a breaking change.
There was a problem hiding this comment.
Constants should be SCREAMING_SNAKE_CASE
EDIT: From what I could tell nowhere used this constant inside the suite. But this constant "may" be accessed from outside the suite. I don't know.
There was a problem hiding this comment.
I think we should disable the cop for this particular case, since we're basically setting up a class here.
There was a problem hiding this comment.
It seems a proper convoluted way of doing things, but I'll alter this change for now and rebase.
|
@luke-hill could you please rebase this on current master? |
|
@luke-hill I think you merged instead of rebasing? Let's wait for Travis right now and then fix that up. |
|
I fixed this up and rebased with the latest master, but yeh now we have like 100 failures regarding environment. I did a merge commit as it's easier than rebasing when your PR is >5 commits. But yeh lord knows what's gone on here. Have we removed something in |
|
I don't have time now, but I can have a deeper look into this later in the week. |
This seems to cause massive suite-wide errors when being renamed, so needs a much deeper investigation
|
I've fixed up the issues and added a warning about that area. In theory that "shouldn't" have broken it, as we shouldn't be accessing a memo's iVar. Still it's now all green and slightly better. It's also RtM if you're happy with it. |
|
Thanks, @luke-hill! |
Summary
Fix up some more rubocop offenses
Details
Some auto-fixed, some manually fixed. Approx 70 failures have been fixed and the config has been regenerated
Motivation and Context
Make the codebase better, one offense at a time.
How Has This Been Tested?
CI
Screenshots (if appropriate):
Types of changes
Checklist: