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

Rollup! #10

Merged
merged 9 commits into from Apr 17, 2018
Merged

Rollup! #10

merged 9 commits into from Apr 17, 2018

Conversation

killercup
Copy link
Collaborator

@killercup killercup commented Apr 17, 2018

Wow, there's a lot of stuff going on in this repo! This is a rollup of #6 and #9 -- and I'm pretty sure also of #8 of which #9 is pretty much a superset.

So, to summarize:

  • This fixes the tests
  • This makes clippy and rustfmt happy
  • This uses Path/PathBuf instead of String in a few places

…and it finally it bumps the version to 0.3 because, even though print_msg now accepts strictly more inputs (you can still pass in a String but now also give it a Path or PathBuf of &str), the output type of handle_dump changed :(

@killercup
Copy link
Collaborator Author

killercup commented Apr 17, 2018

r? @badboy @skade @yoshuawuyts

roll up premium

@badboy
Copy link

badboy commented Apr 17, 2018

r+ for the Path{,Buf} stuff!

Copy link
Collaborator

@spacekookie spacekookie left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@@ -99,5 +115,5 @@ pub fn handle_dump(panic_info: &PanicInfo) -> Result<String, FailError> {
}

let report = Report::new(Method::Panic, expl);
return report.persist();
report.persist()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a fan of explicit return statements but I think I'm in the minority there in the Rust community 😜

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Who cares what the rust community thinks – clippy doesn't like it :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

Al̶l̛ ̨hai̷l҉ th̵è įǹfa͝l҉lib̢le cĺi̴p͟p̵y̸

Copy link
Collaborator

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@yoshuawuyts
Copy link
Collaborator

@killercup as a side note: I don't usually tend to bump versions in a PR, but instead wait until after merge. Usually so it can be labeled & tagged in a separate commit / shows up in the commit log as standalone. Nothing to block this on though.

@killercup
Copy link
Collaborator Author

killercup commented Apr 17, 2018

@yoshuawuyts ah no, that's fine, I just saw your comment in #6 about the version bump and went ahead. Let me remove that commit here. We should probably go over the docs before a release anyway

Update: @skade already delivered: #11

@killercup killercup merged commit d862329 into rust-cli:master Apr 17, 2018
epage added a commit that referenced this pull request Dec 4, 2023
chore(config): migrate renovate config
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

5 participants