Skip to content
This repository has been archived by the owner. It is now read-only.

/rustdoc

Error-chain -> failure #202

Merged
merged 4 commits into from Nov 27, 2017

Conversation

Reviewers

@euclio euclio

@withoutboats withoutboats

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@steveklabnik @mgattozzi @euclio @withoutboats
@steveklabnik
Copy link
Owner

steveklabnik commented Oct 25, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

mgattozzi commented Oct 25, 2017

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

euclio reviewed Oct 25, 2017
View changes

src/lib.rs Outdated
@@ -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()

This comment has been minimized.

Sign in to view
Copy link
@euclio

euclio Oct 25, 2017

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@steveklabnik

steveklabnik Oct 27, 2017

Author Owner

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

This comment has been minimized.

Sign in to view
Copy link
@euclio

euclio Oct 27, 2017 •
edited

Contributor

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 😂

This comment has been minimized.

Sign in to view
Copy link
@withoutboats

withoutboats Oct 28, 2017

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

euclio reviewed Oct 25, 2017
View changes

src/lib.rs Outdated
@@ -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(|_|

This comment has been minimized.

Sign in to view
Copy link
@euclio

euclio Oct 25, 2017

Contributor

Same here.
@euclio

This comment has been minimized.

Sign in to view
Copy link
Contributor

euclio commented Oct 25, 2017

This looks interesting!
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Oct 26, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

mgattozzi commented Oct 26, 2017

Awesome!
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 2, 2017

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

@steveklabnik steveklabnik force-pushed the failure branch from 726d4d0 to eb88151 Nov 2, 2017

@withoutboats

This comment has been minimized.

Sign in to view
Copy link

withoutboats commented Nov 2, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 3, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

euclio commented Nov 6, 2017 •
edited

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 steveklabnik force-pushed the failure branch 2 times, most recently from ea4870c to 5bace42 Nov 22, 2017

@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

euclio commented Nov 22, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

ah, that makes perfect sense.

ill dig in.

@steveklabnik steveklabnik force-pushed the failure branch from 5bace42 to 9dadf6c Nov 22, 2017

@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

oh wait looks like a bad rebase....

@steveklabnik steveklabnik force-pushed the failure branch from 9dadf6c to 0e72672 Nov 22, 2017

@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

Got master passing, this seems to reintroduce it. Hmmmmmmmmm

@steveklabnik steveklabnik force-pushed the failure branch from 0e72672 to 6936fef Nov 22, 2017

@steveklabnik
error-chain -> failure 
failure is becoming the de-facto error handling crate.
Loading status checks…
06ed65c

@steveklabnik steveklabnik force-pushed the failure branch from 6936fef to 06ed65c Nov 22, 2017

@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

@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

euclio reviewed Nov 22, 2017
View changes

tests/source.rs Outdated

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

This comment has been minimized.

Sign in to view
Copy link
@euclio

euclio Nov 22, 2017

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@steveklabnik

steveklabnik Nov 22, 2017

Author Owner

lol oops
@euclio

euclio reviewed Nov 22, 2017
View changes

tests/source.rs Outdated

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

This comment has been minimized.

Sign in to view
Copy link
@euclio

euclio Nov 22, 2017

Contributor

This should be if args.len() != 1
@steveklabnik
fix @euclio feedback
Loading status checks…
efa1bdc
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

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

euclio reviewed Nov 22, 2017
View changes

tests/source.rs Outdated
@@ -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());

This comment has been minimized.

Sign in to view
Copy link
@euclio

euclio Nov 22, 2017

Contributor

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.

This comment has been minimized.

Sign in to view
Copy link
@steveklabnik

steveklabnik Nov 22, 2017

Author Owner

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

euclio reviewed Nov 22, 2017
View changes

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 comment has been minimized.

Sign in to view
Copy link
@euclio

euclio Nov 22, 2017

Contributor

This can be ok_or_else
@euclio

This comment has been minimized.

Sign in to view
Copy link
Contributor

euclio commented Nov 22, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

euclio commented Nov 22, 2017

Yeah, I was confused by that too. Does downcasting the cause to jmespath::JmespathError work?
@steveklabnik
flip it, reverse it
Loading status checks…
3e78e03
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

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
maybe this?
Loading status checks…
dcf4107
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 22, 2017

@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

This comment has been minimized.

Sign in to view
Copy link
Contributor

euclio commented Nov 22, 2017

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 steveklabnik merged commit 7a62956 into master Nov 27, 2017
4 checks passed

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Owner Author

steveklabnik commented Nov 27, 2017

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

@steveklabnik steveklabnik deleted the failure branch Nov 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.