Stable and slim #1

killercup · 2018-04-15T11:27:25Z

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.

Also adds a nightly feature flag that will automatically be picked up by docs.rs to build nice docs.

You can run `cargo clippy` locally to get the same effect. I've also taken the liberty to nail down the rustfmt version to use, so we can update it explicitly. (This is the same CI setup that assert_cli uses.)

Improves compile times

This should make it compatible with windows consoles.

killercup · 2018-04-15T11:28:29Z

(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 · 2018-04-15T14:25:13Z

@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!

yoshuawuyts · 2018-04-15T14:25:56Z

src/lib.rs

-
-use console::style;
-use failure::Error;
+#![cfg_attr(feature = "nightly", deny(missing_docs))]


This should be enabled on every channel I reckon

I can't enable missing_docs on stable because then it'll complain about missing module docs! (As loading the Readme is only done in nightly.)

Ahaha, yeah alright - fair enough! There's hacks we could do, but let's not do them.

yoshuawuyts · 2018-04-15T14:27:42Z

src/lib.rs


 /// Catch any error handlers that occur, and
 // Cargo env vars available:
 // https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates
-pub fn catch_unwind<F: FnOnce() -> Result<(), Error>>(f: F) {
+pub fn catch_unwind<F: FnOnce() -> Result<(), Box<Error>>>(f: F) {


Curious: why did you opt to replace failure::Error with Box<Error>? The former should guarantee we always get a stack trace, which I figured would be useful for the reporting.

Oh, is this why you used failure? I'm not sure that's what's happening: Failure will only give you stack traces for errors that occur in the catch_unwind code -- everything else is part of the panic_info, right?

yeah, that's totally true! - the way I'm envisioning this though is that this would run in the outer most scope of fn main() to catch everything that happens inside of it. And then like with quicli it also handles errors that have been returned. Does that make sense?

Hmmm. I see what you mean, but I don't think this is how it works. Failure's backtrace will only collect traces when an error is actually propagated -- but when you panic!, that doesn't happen (instead, Rust will unwind the stack; unless you set `panic = "abort").

I'm not an authority on how to really solve this, though!

killercup · 2018-04-15T14:31:25Z

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 · 2018-04-15T16:33:23Z

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 😁

killercup · 2018-04-15T18:54:25Z

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 · 2018-04-15T19:06:39Z

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

epage · 2018-04-15T19:11:29Z

.travis.yml

+  - rust: nightly-2018-04-13
+    env: CLIPPY_VERS="0.0.193"
+    before_script: |
+      [[ "$(cargo +nightly-2018-04-13 clippy --version)" != "$CLIPPY_VERS" ]] && \


This actually isn't needed. I know I recommended it in the past for assert_cli but have learned more about travis as part of documenting it for crate-ci.

Travis will ignore the caches on environment variable changes and since the version is kept in an environment variable, it will auto-clear.

Oh, interesting! Should be depend on Travis' behavior here, though? At least this way it should also work for other CI systems, right?

Except the file is travis specific. As we document other CIs, we can note the special things that might need to be done in them. Overall, I like Travis' approach; I wish I had that on Jenkins at work.

yoshuawuyts · 2018-04-15T20:59:46Z

@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!

and choose a nightly that actually exists.

killercup · 2018-04-16T08:50:31Z

Yay, CI works!

killercup · 2018-04-16T14:05:28Z

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

This commit cleans up the code quite significantly and gates certain features behind `nightly` macros to still allow complication on stable Rust.

chore(deps): update msrv to v1.65.0

killercup added 4 commits April 15, 2018 13:15

Make it compile on stable

68dcef5

Also adds a nightly feature flag that will automatically be picked up by docs.rs to build nice docs.

Make clippy a CI-only dependency

e7e074a

You can run `cargo clippy` locally to get the same effect. I've also taken the liberty to nail down the rustfmt version to use, so we can update it explicitly. (This is the same CI setup that assert_cli uses.)

Get rid of all dependencies for now

65b2c46

Improves compile times

Use termcolor for colored output

631a9d6

This should make it compatible with windows consoles.

killercup added 2 commits April 15, 2018 13:34

Set up some kind of error handling for the hook

315b056

rustfmt

5fe3412

yoshuawuyts reviewed Apr 15, 2018

View reviewed changes

epage reviewed Apr 15, 2018

View reviewed changes

spacekookie mentioned this pull request Apr 15, 2018

Metadata for error message not parsed properly #2

Closed

yoshuawuyts mentioned this pull request Apr 15, 2018

Report #4

Closed

Bump clippy

9f79f96

and choose a nightly that actually exists.

killercup force-pushed the stable-and-slim branch from f43e9a5 to 9f79f96 Compare April 16, 2018 08:41

Make clippy happy

a83cbd1

spacekookie mentioned this pull request Apr 16, 2018

Big Meta Merge #5

Merged

yoshuawuyts merged commit b90ea3b into rust-cli:master Apr 16, 2018

epage added a commit to epage/human-panic that referenced this pull request May 19, 2023

Merge pull request rust-cli#1 from epage/renovate/rust-1.x

29b981c

chore(deps): update msrv to v1.65.0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stable and slim #1

Stable and slim #1

killercup commented Apr 15, 2018 •

edited

killercup commented Apr 15, 2018

yoshuawuyts commented Apr 15, 2018

yoshuawuyts Apr 15, 2018

killercup Apr 15, 2018

yoshuawuyts Apr 15, 2018

yoshuawuyts Apr 15, 2018

killercup Apr 15, 2018

yoshuawuyts Apr 15, 2018

killercup Apr 15, 2018

killercup commented Apr 15, 2018

yoshuawuyts commented Apr 15, 2018

killercup commented Apr 15, 2018

epage commented Apr 15, 2018

epage Apr 15, 2018

killercup Apr 15, 2018

epage Apr 15, 2018

yoshuawuyts commented Apr 15, 2018

killercup commented Apr 16, 2018 •

edited

killercup commented Apr 16, 2018

Stable and slim #1

Stable and slim #1

Conversation

killercup commented Apr 15, 2018 • edited

Semver Changes

killercup commented Apr 15, 2018

Checklist

yoshuawuyts commented Apr 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup commented Apr 15, 2018

yoshuawuyts commented Apr 15, 2018

killercup commented Apr 15, 2018

epage commented Apr 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts commented Apr 15, 2018

killercup commented Apr 16, 2018 • edited

killercup commented Apr 16, 2018

killercup commented Apr 15, 2018 •

edited

killercup commented Apr 16, 2018 •

edited