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

Only display message for humans (implementing #48) #50

Merged
merged 2 commits into from
Oct 4, 2018
Merged

Conversation

spacekookie
Copy link
Collaborator

@spacekookie spacekookie commented Oct 4, 2018

This is a 🙋 feature.

Overview

The general rationale for this is documented in #48.
This PR implements the required behaviour. A human-panic
message will only be displayed if two conditions are met

  1. The program is not running in DEBUG mode
  2. RUST_BACKTRACE is not set as an env variable

This PR also adjusts the tests to check for both present
and non-present human-panic messages.


There are two things missing

  • Properly document this change!
  • A test that checks for the RUST_BACKTRACE override
    behaviour when running in RELEASE mode. This is currently
    not easy to do with assert_CLI (there might be more PRs 🙂)

Checklist

  • tests pass
  • tests and/or benchmarks are included
  • documentation is changed or added

Semver change

Now, this is a matter of how you define a version breakage.
SemVer is ambigous when it comes to what is and isn't a breakage,
for example, if people are depending on human-panic messages to be
present to parse things from logs then any tool that updates
human-panic while we are changing behaviour will break.

In the code, this is at most a minor bump. But seeing as we're
also looking at things to add for a 2.0 release, we might want to
put it on the tracking issue (#46).

Either way, we need to document this change because it might have
a large impact on people who depend on the stdout message to be
present.

@spacekookie spacekookie merged commit 80867e1 into master Oct 4, 2018
@spacekookie spacekookie deleted the dont-panic branch October 4, 2018 17:41
@spacekookie spacekookie mentioned this pull request Oct 4, 2018
7 tasks
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.

None yet

1 participant