-
Notifications
You must be signed in to change notification settings - Fork 9
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
Return non-zero exit codes when something goes wrong (WIP) #54
base: master
Are you sure you want to change the base?
Conversation
lib/pulsar/executor.rb
Outdated
module Pulsar | ||
module Executor | ||
def self.sh(cmd) | ||
_stdin, _stdout, _stderr, _wait_thr = Open3.popen3(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use prefix _ for a variable that is used.
8d434c4
to
a2b58ad
Compare
lib/pulsar/cli.rb
Outdated
@@ -91,5 +104,7 @@ def load_option_or_env!(option) | |||
|
|||
option_value | |||
end | |||
|
|||
def self.exit_on_failure?; true; end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private (on line 84) does not make singleton methods private. Use private_class_method or private inside a class << self block instead.
This close #52
Some references around the web (like this) tells that is a standard to return a value of
0
when the command performed successfully and a value different form0
otherwise.So this is the
exit_status
table for our project:IMPORTANT
I think this PR will never solve the related issue: there is an unresolvable conflict between
Kernel::exit
andsimplecov
gem. Maybe the cause is related toat_exit
callback used by simplecov. I noticed many unpredictable weirds behaviors. I tried to keep tests green and full coverage, with many tricks and workarounds but I lost the challenge.NB. The statement with
Kernel::exit
method is not covered by default by ruby built-in coverage mechanism, so in any case we'll get a100%
coverage result when using this method.