Skip to content

Conversation

@neomilium
Copy link
Contributor

On top of #202 , this PR fixes exit status code on failures.

Previously, msync often returns 0 when errors occurred, this PR sets it to 1 and turn puts into warn in order to output the error on stderr instead of stdout.

@neomilium neomilium marked this pull request as draft December 21, 2020 15:06
@neomilium
Copy link
Contributor Author

Mark this as draft waiting for PR #202 to be merged first.

@neomilium neomilium force-pushed the fix-exit-code branch 2 times, most recently from 3e594e6 to 9d36454 Compare December 21, 2020 22:46
Long-time issues on Thor related to exit status code cause msync to return a inappropriate exit code.

As example: rails/thor#244
@neomilium neomilium marked this pull request as ready for review January 15, 2021 10:11
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Small nit, otherwise 👍

.to_a
else
$stdout.puts "#{local_template_dir} does not exist." \
warn "#{local_template_dir} does not exist." \
Copy link
Member

Choose a reason for hiding this comment

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

This is Kernel.warn, right? I'm not sure if that's the correct thing to do. Other places in this file use $stdout.puts and $stderr.puts so I'd prefer to remain consistent. In this case I think it should be $stderr.puts. The same below.

The reason is that you can disable all warnings when you run ruby -W0 which I sometimes do if there's some deprecation warning. Some projects generate them because they're fully Ruby 2.7 clean. That would hide this output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My wish was to write clean code and rubocop recent version advises to turn $stderr.puts int warn but you're right: warn can be disabled using ruby -W0.
So, I will revert the turn from $stderr.puts into warn as its more an error than a warning.
Thanks for notice.

Source: https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/StderrPuts

These tests were successful for wrong reasons!
This commit fixes the relevant scenarios, by setting correctly the requirements (ie. message and moduleroot)
@ekohl ekohl merged commit 1b5284e into voxpupuli:master Jan 18, 2021
@ekohl
Copy link
Member

ekohl commented Jan 18, 2021

Thanks!

@neomilium neomilium deleted the fix-exit-code branch January 18, 2021 16:33
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.

3 participants