Stable and slim - Githubissues

rust-cli / human-panic

Panic messages for humans.

https://docs.rs/human-panic

Apache License 2.0

1.66k stars 65 forks source link

Stable and slim #1

Closed killercup closed 6 years ago

killercup commented 6 years ago

This is a pretty general refactoring of the code.

The goal of this PR is to make this a crate that is useful in more general contexts. For this, I've:

put the nightly-only external_doc feature behind a feature flag
changed how clippy is used on CI by installing cargo-clippy in a new travis builder
removed failure and unindent as dependencies
replaced console with termcolor which is known to work on Windows

For more information, see the individual commit messages.

Semver Changes

This does not change observable behavior.

killercup commented 6 years ago

(wrote this as a comment to prevent it from becoming part of the merge commit message)

Checklist

[ ] tests pass -- well, I've change the CI script. Let's see what happens. Update: Did you set up travis yet?

yoshuawuyts commented 6 years ago

@killercup niceee!

Some quick notes:

loving the general refactor; didn't know this was possible, and digging it a lot.
travis is setup now!
not too sure about the clippy pinning; I kind of like always having the latest, despite the possibility it might fail sporadically. Also allows people to run Clippy locally, as opposed to only through CI - if possible I'd like to keep it as-is for now.

Overall really liking it!

killercup commented 6 years ago

not too sure about the clippy pinning; I kind of like always having the latest, despite the possibility it might fail sporadically.

I can change it to always install the latest version on the latest nightly and make this an allowed failure on Travis if you want. (I'd personally prefer the stability because allowed failures tend to go unnoticed.)

Also allows people to run Clippy locally, as opposed to only through CI - if possible I'd like to keep it as-is for now.

You can do cargo install clippy locally and then run cargo clippy -- also works on project without any clippy setup :)

yoshuawuyts commented 6 years ago

You can do cargo install clippy locally and then run cargo clippy -- also works on project without any clippy setup :)

Haha, yeah - but I reallllly just want to run it whenever cargo test is done, so that people only need to think of a single command during development. I'm thinking this will be the type of module we write once, and then it's done. Maybe some minor maintenance over the years, but nothing major.

The less work there is in a toolchain, the better :grin:

killercup commented 6 years ago

Haha, yeah - but I reallllly just want to run it whenever cargo test is done, so that people only need to think of a single command during development.

I'd love to have that as well! Alas, that is in stark conflict with "this still compiles in a year" right now :(

It's a trade-off. Just tell me what you want to have and I'll implement it.

epage commented 6 years ago

I recommend pinning clippy in the CI. I have found that contributors get frustrated when a CI arbitrarily breaks on them and pinning as a big help for this. Pinning also makes it possible to cache your clippy install to reduce CI times and load on these CI servers that are gifted to us. For more, see https://crate-ci.github.io/pr/clippy.html

yoshuawuyts commented 6 years ago

@killercup sure, let's go with what you proposed - it seems most folks in this thread seem in favor of that, so good with me!

killercup commented 6 years ago

Yay, CI works!

killercup commented 6 years ago

Fixed the clippy issue. Let me know what else you need me to change.