Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add context to errors #24

Merged
merged 7 commits into from Jun 4, 2016

Conversation

Projects
None yet
2 participants
@tailhook
Copy link
Owner

commented May 17, 2016

This issue was already brought as #4. This one tracks current implementation in branch context2.

You can try it with:

[dependencies.quick-error]
git = "http://github.com/tailhook/quick-error"
branch = "context2"

Example usage

Todo list:

  • Basic support for tuple enum members
  • Experiment with generics (AsRef<X> only, can we provide arbitrary generics somehow?)
  • Fix support for struct enum members
  • Add unit tests
  • Add documentation
  • Add Debug implementation, make sure that .unwrap() and .expect work
@tailhook

This comment has been minimized.

Copy link
Owner Author

commented May 17, 2016

I'm going to write an article about this. But, @colin-kiegel, @EPashkin, may be you have some quick comments?

@EPashkin

This comment has been minimized.

Copy link

commented May 17, 2016

Look good, but not applicable to my case due identical source types for separate error cases (EPashkin/gir@025bba7), so I can't "judge" this PR

@tailhook

This comment has been minimized.

Copy link
Owner Author

commented May 17, 2016

@EPashkin, I have two observations:

  1. Why Toml error is a string? (And options case?) I guess in the perfect world they need to be specific structures
  2. You can either wrap error into itermediate type or wrap context into intermediate type. Latter is a a little bit shorter.

While not so short any more, in the error handling code it looks nicer than the original anyway: have less parenthesis, fit single line, no repeating to_path_buf(). In the error definition code, you just need a single wrapper structure (one-liner) and no traits on it. You need to import the structure every time, though.

For more complex cases, a special structure which represents context may be useful enough on it's own, for example to keep multiple context parameters defined in single place.

@tailhook

This comment has been minimized.

Copy link
Owner Author

commented May 17, 2016

@EPashkin, just to be sure, the tuple solution in #4 has this problem too, right?

@EPashkin

This comment has been minimized.

Copy link

commented May 17, 2016

We decide that Toml error is string because Toml's Parser has many errors, returned in non standard way and required Parser instance to show it right.

@EPashkin

This comment has been minimized.

Copy link

commented May 17, 2016

Yes, tuple solution has same problem, so we add error constructors and call its directly

@tailhook

This comment has been minimized.

Copy link
Owner Author

commented May 17, 2016

We decide that Toml error is string because Toml's Parser has many errors, returned in non standard way and required Parser instance to show it right.

Okay. I think you could put Vec<ParserError> instead of a string there. Or maybe Vec<(Pos, ParserError)> (withPos {line_no: usize, col: usize} or tuple or whatever). And I think it would rather improve the code.

Anyway, let's look at other options:

  1. Provide an extension trait for each structure option. I.e. by importing MyResultExt you would instead of .context(path) do
    .err_context_io(path) in my example. Or in gir case .err_toml(path) (trying to find shorter convention) vs .context(TomlErr(path)) in current implementation
  2. Choose error type as an argument .context(path, Error::Toml) (not sure if this is implementable)
  3. Add some facility to make helper functions .map_err(|e| Error::toml(e, &path))
  4. Status quo: .map_err(|e| Error::Toml(e, path.to_path_buf()))

Any other ideas?

@EPashkin

This comment has been minimized.

Copy link

commented May 17, 2016

About Toml: as error require Parser instance to show correctly we can just store Parser in your error and do processing in description() or fmt() but just don't like it.

@tailhook

This comment has been minimized.

Copy link
Owner Author

commented May 17, 2016

About Toml: as error require Parser instance to show correctly we can just store Parser in your error and do processing in description() or fmt() but just don't like it.

That's understandable. I just think you could convert error vector into vector of tuples (Pos, Error) using the parser and drop the parser, rather that converting straight away to String.

@EPashkin

This comment has been minimized.

Copy link

commented May 17, 2016

I currently don't have other idea about options.
I like first, but not sure that it has clarity.
3th need good generalization support.

@tailhook

This comment has been minimized.

Copy link
Owner Author

commented Jun 3, 2016

Okay, I'm going to merge this into master, and release a 1.1. Any objections?

ping @colin-kiegel

@tailhook

This comment has been minimized.

Copy link
Owner Author

commented Jun 3, 2016

By the way here is the article. I'll probably write another one with more complex usage examples, but this pull request doesn't have to wait.

@tailhook tailhook merged commit 4c2e467 into master Jun 4, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tailhook tailhook deleted the context2 branch Jun 4, 2016

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