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 error with devel install from erroneuous Rubocop fix #266

Merged
merged 1 commit into from
Aug 1, 2016

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Jul 28, 2016

No description provided.

@@ -96,7 +96,7 @@ def run_installer(command)

def syscall(command)
system(command)
$CHILD_STATUS.zero?
$CHILD_STATUS.to_i.zero?
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want this to return true when $CHILD_STATUS is nil? I ask because previously ($CHILD_STATUS == 0) it would return false for nil.

Copy link
Member

Choose a reason for hiding this comment

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

Why not just $?.success?

@ehelms
Copy link
Member Author

ehelms commented Aug 1, 2016

Updated based on @stbenjam suggestion

@stbenjam
Copy link
Member

stbenjam commented Aug 1, 2016

Lol sorry, rubocop doesn't like it, but I think rubocop's suggestion is bad and maybe we can just turn it off.

1.8.7-head :007 > system('ls')
[...snip...]
1.8.7-head :008 > $CHILD_STATUS.success?
NoMethodError: undefined method `success?' for nil:NilClass
    from (irb):8
    from /usr/bin/irb:11:in `<main>'
1.8.7-head :009 > $?.success?
 => true 

@ehelms
Copy link
Member Author

ehelms commented Aug 1, 2016

Let's see if its happy now...

@stbenjam
Copy link
Member

stbenjam commented Aug 1, 2016

ACK

@daviddavis
Copy link
Contributor

daviddavis commented Aug 1, 2016

Looks like this is caused by not requiring 'English'. See here:

rubocop/rubocop#1747

[5] pry(main)> require 'English'
=> true
[6] pry(main)> system('ls')
[...]
[7] pry(main)> $CHILD_STATUS.success?
=> true

@stbenjam
Copy link
Member

stbenjam commented Aug 1, 2016

I still prefer $?

@daviddavis
Copy link
Contributor

daviddavis commented Aug 1, 2016

Yea, I think $? is clear because I see it so much in the terminal. However, some of the others like $', $;, etc, I would have to look up. I wish rubocop would let you whitelist particular ones though like $?. I wouldn't mind disabling this cop.

@ehelms
Copy link
Member Author

ehelms commented Aug 1, 2016

Good to merge this as is? I'll most likely be turning a lot of this into
roles and playbooks so it shouldn't be around for too long.

On Aug 1, 2016 12:54 PM, "David Davis" notifications@github.com wrote:

I think $? is clear because I see it so much in the terminal. However,
some of the others like $', $;, etc, I would have to look up
http://ruby-doc.org/stdlib-2.0.0/libdoc/English/rdoc/English.html. I
wish rubocop would let you whitelist particular ones though like $?. I
wouldn't mind disabling this rubocop.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#266 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAR585lA8NaKgQNak-vN3MD5gr3F8crhks5qbiSzgaJpZM4JXveU
.

@daviddavis
Copy link
Contributor

ACK from me.

@ehelms ehelms merged commit d41fb79 into theforeman:master Aug 1, 2016
johnpmitsch pushed a commit to johnpmitsch/forklift that referenced this pull request Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants