Start migration to the failure crate
#4795
Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
|
☔ The latest upstream changes (presumably #4788) made this pull request unmergeable. Please resolve the merge conflicts. |
alexcrichton
force-pushed
the
failure
branch
from
e37aa32
to
2fd482c
Compare
|
☔ The latest upstream changes (presumably #4828) made this pull request unmergeable. Please resolve the merge conflicts. |
alexcrichton
force-pushed
the
failure
branch
from
2fd482c
to
a8a5dc9
Compare
|
☔ The latest upstream changes (presumably #4818) made this pull request unmergeable. Please resolve the merge conflicts. |
alexcrichton
force-pushed
the
failure
branch
from
a8a5dc9
to
7b59a31
Compare
src/cargo/util/errors.rs
Outdated
| } | ||
| } | ||
| } | ||
| // impl Error for CliError { |
| links { | ||
| CrateRegistry(registry::Error, registry::ErrorKind); | ||
| } | ||
| pub trait CargoResultExt<T, E> { |
There was a problem hiding this comment.
Why still have this instead of using failure's ResultExt?
There was a problem hiding this comment.
Oh this is purely transitionary, I wanted to make sure we got past the test at least before blanket renaming chain_err to with_context everywhere. I plan on doing that change after this PR lands (just to cut down on merge conflicts hopefully)
| @@ -76,7 +80,7 @@ pub fn read(path: &Path) -> CargoResult<String> { | |||
| } | |||
|
|
|||
| pub fn read_bytes(path: &Path) -> CargoResult<Vec<u8>> { | |||
| (|| -> CargoResult<_> { | |||
| let res = (|| -> CargoResult<_> { | |||
There was a problem hiding this comment.
Could also .map_err(Into::into) instead of this ?; Ok(res) stuff.
| impl CargoError { | ||
| pub fn into_internal(self) -> Self { | ||
| CargoError(CargoErrorKind::Internal(Box::new(self.0)), self.1) | ||
| impl Fail for Internal { |
There was a problem hiding this comment.
Will keep in mind that this can't be derived and see if I could make it so it could be. :-)
src/cargo/util/errors.rs
Outdated
| description("failed to get a 200 response") | ||
| display("failed to get 200 response from `{}`, got {}", url, code) | ||
| } | ||
| // error_chain! { |
There was a problem hiding this comment.
Also could probably be deleted?
| @@ -31,7 +31,8 @@ impl<'cfg> Registry for ReplacedSource<'cfg> { | |||
| }).chain_err(|| { | |||
| format!("failed to query replaced source {}", | |||
| self.to_replace) | |||
| }) | |||
| })?; | |||
There was a problem hiding this comment.
could be .map_err(Into::into) if you don't want to do this two statement thing.
There was a problem hiding this comment.
Indeed yeah! I'm not necessarily a huge fan of that, but I found the two-statement return wasn't so bad.
There was a problem hiding this comment.
I have no opinion :-)
alexcrichton
force-pushed
the
failure
branch
from
7b59a31
to
595dfc5
Compare
|
mk, updated! |
Cargo.toml
Outdated
| @@ -24,7 +24,7 @@ crypto-hash = "0.3" | |||
| curl = "0.4.6" | |||
| docopt = "0.8.1" | |||
| env_logger = "0.4" | |||
| error-chain = "0.11.0" | |||
| failure = "0.1" | |||
There was a problem hiding this comment.
You use bail!, so this should be 0.1.1 in case some monster tries to say failure = "=0.1.0".
|
@bors r+ |
|
📌 Commit 595dfc5 has been approved by |
|
⌛ Testing commit 595dfc51ec4e92fcc178f991b38178074a01ab72 with merge 026e91333995ee1c23c22fe5dee4b46409d9affd... |
This commit is the initial steps to migrate Cargo's error handling from the `error-chain` crate to the `failure` crate. This is intended to be a low-cost (in terms of diff) transition where possible so it's note "purely idiomatic `failure` crate" just yet. The `error-chain` dependency is dropped in this commit and Cargo now canonically uses the `Error` type from the `failure` crate. The main last remnant of `error-chain` is a custom local extension trait to use `chain_err` instead of `with_context`. I'll try to follow up with a commit that renames this later but I wanted to make sure everything worked first! (and `chain_err` is used practically everywhere). Some minor tweaks happened in the tests as I touched up a few error messages here and there but overall the UI of Cargo should be exactly the same before and after this commit.
alexcrichton
force-pushed
the
failure
branch
from
595dfc5
to
37cffbe
Compare
|
@bors: r=withoutboats |
|
📌 Commit 37cffbe has been approved by |
Start migration to the `failure` crate This commit is the initial steps to migrate Cargo's error handling from the `error-chain` crate to the `failure` crate. This is intended to be a low-cost (in terms of diff) transition where possible so it's note "purely idiomatic `failure` crate" just yet. The `error-chain` dependency is dropped in this commit and Cargo now canonically uses the `Error` type from the `failure` crate. The main last remnant of `error-chain` is a custom local extension trait to use `chain_err` instead of `with_context`. I'll try to follow up with a commit that renames this later but I wanted to make sure everything worked first! (and `chain_err` is used practically everywhere). Some minor tweaks happened in the tests as I touched up a few error messages here and there but overall the UI of Cargo should be exactly the same before and after this commit.
|
☀️ Test successful - status-appveyor, status-travis |
This commit is the initial steps to migrate Cargo's error handling from the
error-chaincrate to thefailurecrate. This is intended to be a low-cost(in terms of diff) transition where possible so it's note "purely idiomatic
failurecrate" just yet.The
error-chaindependency is dropped in this commit and Cargo now canonicallyuses the
Errortype from thefailurecrate. The main last remnant oferror-chainis a custom local extension trait to usechain_errinstead ofwith_context. I'll try to follow up with a commit that renames this later butI wanted to make sure everything worked first! (and
chain_erris usedpractically everywhere).
Some minor tweaks happened in the tests as I touched up a few error messages
here and there but overall the UI of Cargo should be exactly the same before and
after this commit.