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

Fix empty checks #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix empty checks #9

wants to merge 1 commit into from

Conversation

tcrouch
Copy link
Contributor

@tcrouch tcrouch commented Oct 31, 2018

Consistently order calls to nil? & empty?, as empty? is undefined for NilClass.

Consistently order calls to nil? & empty?; use boolean || operator
@timdiggins
Copy link

This looks good -- or is not shortcircuiting because lower precedence -- it's not a boolean operator really (http://www.virtuouscode.com/2010/08/02/using-and-and-or-in-ruby/)

In the test suite this changes a "no implicit conversion of nil into String" error in the codebase into a RestClient::Unauthorized: 401 Unauthorized (which is progress).

Can it be merged?

Alternatively could use &.empty? if you don't mind excluding < ruby 2.3 (which seems like a good shout as ruby 2.2 is EOL https://www.ruby-lang.org/en/downloads/branches/)

Would need to document that somewhere (also a good shout).

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