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

Rubocop: Your move, Creep #1283

Merged
merged 10 commits into from
Nov 21, 2016
Merged

Rubocop: Your move, Creep #1283

merged 10 commits into from
Nov 21, 2016

Conversation

jaredmoody
Copy link
Contributor

Low hanging rubocop fixes for #945

I’m not 100% sure on the Lint/EmptyWhen fix if someone can double check that, I’d appreciate it.

class Learning
class App
include TextPlugin::App
end
Copy link
Member

Choose a reason for hiding this comment

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

This seems to break the tests, cause Learning is a module not a class :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks :P

when Empty, Continue
else @line = ""
end
@line = "" unless e == Empty || e == Continue
Copy link
Member

Choose a reason for hiding this comment

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

TBH this seems wrong?! o_O Imo it should be Empty === e if we wanted to literally translate the case statement to an if. I'd be shocked to find such a big bug in rubocop though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rubocop was complaining about the empty when, but I made the change here - we're through all the auto-correctable cops now. I wasn't sure about this change though, so I mentioned it in the PR comments. Thanks for catching that I should have used ===. How do you want this to look though, would this be better?

@line = "" unless [Empty, Continue].include?(e.class)

Copy link
Member

Choose a reason for hiding this comment

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

I think we really need an is_a? check to work with subclassing but yes something else would be better than currently :)

Copy link
Member

@PragTob PragTob left a comment

Choose a reason for hiding this comment

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

LGTM in general, there is the test failure and the (imo) wrong conversion of the empty when conversion...

@@ -17,7 +17,7 @@ def _position(left, top)
gui.update_position if gui && gui.respond_to?(:update_position)
end

# displace(left: a number, top: a number) » self
# displace(left: a number, top: a number) >> self
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -38,8 +38,10 @@

# move the @comp paddle, speed based on `compuspeed` variable
@comp.left +=
if nx + (ball_diameter / 2) > @comp.left + paddle_size; compuspeed
elsif nx < @comp.left; -compuspeed
Copy link
Member

Choose a reason for hiding this comment

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

wow that was ugly :D

@PragTob
Copy link
Member

PragTob commented Nov 20, 2016

Rubocop bumms out...

samples/expert-irb-adjusted.rb:47:25: C: Avoid the use of the case equality operator ===.

    @line = "" unless e === Empty || e === Continue

                        ^^^

samples/expert-irb-adjusted.rb:47:40: C: Avoid the use of the case equality operator ===.

    @line = "" unless e === Empty || e === Continue

                                       ^^^

samples/expert-irb.rb:47:25: C: Avoid the use of the case equality operator ===.

    @line = "" unless e === Empty || e === Continue

                        ^^^

samples/expert-irb.rb:47:40: C: Avoid the use of the case equality operator ===.

    @line = "" unless e === Empty || e === Continue

                                       ^^^

I guess we could substitute it through an e.is_a?(clazz)

@jaredmoody
Copy link
Contributor Author

OK, used is_a? to fix up that exception comparison and fixed that module definition.

@PragTob
Copy link
Member

PragTob commented Nov 21, 2016

Thank you!

@PragTob PragTob merged commit afa7b4e into shoes:master Nov 21, 2016
@jaredmoody jaredmoody deleted the rubocop branch December 3, 2017 01:02
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