Error-chain -> failure #202

steveklabnik · 2017-10-25T20:53:18Z

This PR ports us from error-chain to @withoutboats' failure crate.

I'm done for the day so there's probably CI failures and such, but I wanted to toss this up there so that everyone can see.

/cc @aturon @alexcrichton

mgattozzi · 2017-10-25T21:15:36Z

@steveklabnik curious is there a reason to move towards that over error-chain?

euclio · 2017-10-25T22:25:47Z

src/lib.rs

@@ -172,9 +181,9 @@ pub fn build(config: &Config, artifacts: &[&str]) -> Result<()> {
            .stderr(Stdio::piped())
            .spawn()
            .map_err(|e| if e.kind() == io::ErrorKind::NotFound {
-                Error::with_chain(e, format!("frontend `{}` not found", frontend))
+                format_err!("frontend `{}` not found", frontend).into()


It looks like you could use chain() to keep the context here?

wait, what's chain()? I don't see it in failure's docs

Looks like it's been renamed to context? It looks like it works slightly differently than chain_err did in error_chain? Hard to say without trying it 😂

Yea I renamed it to context. You could do e.context(format!(...)).into() here.

euclio · 2017-10-25T22:26:07Z

src/lib.rs

@@ -196,8 +205,8 @@ pub fn build(config: &Config, artifacts: &[&str]) -> Result<()> {

 /// Run all documentation tests.
 pub fn test(config: &Config) -> Result<()> {
-    let doc_json = File::open(config.documentation_path()).chain_err(
-        || "could not find generated documentation",
+    let doc_json = File::open(config.documentation_path()).map_err(|_|


euclio · 2017-10-25T22:26:32Z

This looks interesting!

steveklabnik · 2017-10-26T01:24:44Z

curious is there a reason to move towards that over error-chain?

It's speculated that this kind of library may be preferable to error-chain overall; I wanted to give it a shot to see what the diff looks like. I think I personally prefer it, but the only way to find out is to give it a try.

mgattozzi · 2017-10-26T13:51:06Z

Awesome!

steveklabnik · 2017-11-02T21:32:12Z

@withoutboats while working on the test part of this PR,

    = note: expected type `std::result::Result<_, failure::Error>`
               found type `std::result::Result<_, failure::Context<InvalidDirective>>`

Do you have to do something specific to use Context? It doesn't implement Error?

withoutboats · 2017-11-02T22:04:58Z

Context implements Fail; to get an Error all Failures have to be cast using ? or into().

steveklabnik · 2017-11-03T14:48:56Z

Thanks boats!

Now our test is failing because of the way we test the context; I suspect that I have to update the macro in some way. More work needed.

euclio · 2017-11-06T18:26:06Z

In this case, it might just be better to check that the cause is the expected type and always return an InvalidDirective failure from parse_test?

steveklabnik · 2017-11-22T15:27:37Z

I've rebased this, and it compiles, but has test failures about not being able to find the source test crates. Hrm.

steveklabnik · 2017-11-22T15:36:46Z

Ohh interesting! https://travis-ci.org/steveklabnik/rustdoc/jobs/305812561

even though #203 passed, it's now failing. So it's likely that PR, not this one.... @euclio any ideas?

euclio · 2017-11-22T15:38:13Z

Failing on master too. Probably related to a change in how the analysis is generated. I did a quick test and it should be fixable by updating rls-analysis and rls-data, though there's an unrelated failure in structs.rs as well.

steveklabnik · 2017-11-22T15:44:01Z

ah, that makes perfect sense.

ill dig in.

steveklabnik · 2017-11-22T16:07:37Z

Updated and rebased, still failing. I must have missed something somewhere...

steveklabnik · 2017-11-22T16:25:12Z

oh wait looks like a bad rebase....

steveklabnik · 2017-11-22T16:46:11Z

Got master passing, this seems to reintroduce it. Hmmmmmmmmm

failure is becoming the de-facto error handling crate.

steveklabnik · 2017-11-22T17:12:14Z

@mgattozzi @euclio okay I think this is ready for a review, maybe you'll spot where I messed up. @withoutboats if you have a second as well, that'd be nice.

euclio · 2017-11-22T20:06:55Z

tests/source.rs


            Directive::Has(regex)
        }
        "matches" => {
-            ensure!(args.len() == 2, "not enough arguments");
+            if args.len() == 2 {


This should be if args.len() != 2

euclio · 2017-11-22T20:07:13Z

tests/source.rs


            Directive::Matches(value)
        }
        "assert" => {
-            ensure!(args.len() == 1, "expected 1 argument");
+            if args.len() == 1 {


This should be if args.len() != 1

steveklabnik · 2017-11-22T20:16:19Z

Awesome, thank you! Of course I messed that up... that leads to just one failure now.

euclio · 2017-11-22T20:42:03Z

tests/source.rs

@@ -391,7 +413,7 @@ fn parse_test(line: &str) -> Option<Result<TestCase>> {
    if let Some(caps) = DIRECTIVE_RE.captures(line) {
        let directive = &caps["directive"];
        let result = parse_directive(directive, &caps["args"], caps.name("negated").is_some())
-            .chain_err(|| ErrorKind::InvalidDirective(line.into()));
+            .map_err(|e| e.context(InvalidDirective { d: line.into() }).into());


I think the context is backwards here. I think that parse_test should return Option<Result<TestCase, InvalidDirective>>. Make sure to set the context of the InvalidDirective error to be the original error. The tests should then check that err.cause() is the expected type.

yeah, this is sorta what i'm thinking as well. been toying with this...

euclio · 2017-11-22T20:43:17Z

tests/source.rs

@@ -401,37 +423,49 @@ fn parse_test(line: &str) -> Option<Result<TestCase>> {
 fn parse_directive(directive: &str, args: &str, negated: bool) -> Result<TestCase> {
    let args = match shlex::split(args) {


This can be ok_or_else

euclio · 2017-11-22T20:50:38Z

Looks good with comments addressed. Any reason you're preferring ok_or over ok_or_else? I don't think clippy likes that.

steveklabnik · 2017-11-22T20:57:37Z

Any reason you're preferring ok_or over ok_or_else? I don't think clippy likes that.

no reason; I'll run clippy too.

now dealing with this fun error:

        thread 'tests::parse_test' panicked at 'unexpected error kind, got InvalidDirective { d: "// @has /test \'[\'" }

invalid JMESPath syntax but expected "InvalidDirective"', tests\source.rs:583:8

hmmmmmmmmmmmmmmmmm

euclio · 2017-11-22T21:01:29Z

Yeah, I was confused by that too. Does downcasting the cause to jmespath::JmespathError work?

steveklabnik · 2017-11-22T21:13:21Z

Yeah, I was confused by that too. Does downcasting the cause to jmespath::JmespathError work?

It doesn't seem to, no matter what I try to cast it to, it doesn't seem to do the right thing. Hrm.

steveklabnik · 2017-11-22T21:23:44Z

@euclio so check out this last commit: if we do make it always return InvalidDirective, then there's no reason to assert that type about it. this is now doing something similar to https://boats.gitlab.io/failure/error-errorkind.html; maybe we should do that exactly?

euclio · 2017-11-22T23:05:45Z

Exactly. Error/ErrorKind could work, but since this is an internal-only error that complexity might not be warranted. We could just make it an enum like here.

steveklabnik · 2017-11-27T18:32:59Z

Time to , if we want to tweak that last error, we can, but I don't think it matters a ton, honestly.

euclio reviewed Oct 25, 2017

View reviewed changes

steveklabnik force-pushed the failure branch from 726d4d0 to eb88151 Compare November 2, 2017 21:36

steveklabnik force-pushed the failure branch 2 times, most recently from ea4870c to 5bace42 Compare November 22, 2017 15:26

steveklabnik force-pushed the failure branch from 5bace42 to 9dadf6c Compare November 22, 2017 16:06

steveklabnik force-pushed the failure branch from 9dadf6c to 0e72672 Compare November 22, 2017 16:25

steveklabnik force-pushed the failure branch from 0e72672 to 6936fef Compare November 22, 2017 16:51

error-chain -> failure

06ed65c

failure is becoming the de-facto error handling crate.

steveklabnik force-pushed the failure branch from 6936fef to 06ed65c Compare November 22, 2017 16:52

euclio reviewed Nov 22, 2017

View reviewed changes

fix @euclio feedback

efa1bdc

euclio reviewed Nov 22, 2017

View reviewed changes

flip it, reverse it

3e78e03

maybe this?

dcf4107

steveklabnik merged commit 7a62956 into master Nov 27, 2017

steveklabnik deleted the failure branch November 27, 2017 18:33

		@@ -401,37 +423,49 @@ fn parse_test(line: &str) -> Option<Result<TestCase>> {
		fn parse_directive(directive: &str, args: &str, negated: bool) -> Result<TestCase> {
		let args = match shlex::split(args) {

Error-chain -> failure #202

Error-chain -> failure #202

Conversation

steveklabnik commented Oct 25, 2017

mgattozzi commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euclio Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euclio commented Oct 25, 2017

steveklabnik commented Oct 26, 2017

mgattozzi commented Oct 26, 2017

steveklabnik commented Nov 2, 2017

withoutboats commented Nov 2, 2017

steveklabnik commented Nov 3, 2017

euclio commented Nov 6, 2017 • edited Loading

steveklabnik commented Nov 22, 2017

steveklabnik commented Nov 22, 2017

euclio commented Nov 22, 2017

steveklabnik commented Nov 22, 2017

steveklabnik commented Nov 22, 2017

steveklabnik commented Nov 22, 2017

steveklabnik commented Nov 22, 2017

steveklabnik commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euclio commented Nov 22, 2017

steveklabnik commented Nov 22, 2017

euclio commented Nov 22, 2017

steveklabnik commented Nov 22, 2017

steveklabnik commented Nov 22, 2017

euclio commented Nov 22, 2017

steveklabnik commented Nov 27, 2017

euclio Oct 27, 2017 •

edited

Loading

euclio commented Nov 6, 2017 •

edited

Loading