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

Follow-up on flex-error integration #946

Open
thanethomson opened this issue Aug 7, 2021 · 1 comment
Open

Follow-up on flex-error integration #946

thanethomson opened this issue Aug 7, 2021 · 1 comment
Labels
enhancement New feature or request

Comments

@thanethomson
Copy link
Member

Follow-up on #923.

I did a quick search through the code from #923 using ripgrep to get a sense of how many times we manually map one error type to another, and this is what I found:

# Not a perfect search, but gives a good rough idea
> rg "map_err\(Error::" --stats
...
122 matches
122 matched lines
36 files contained matches
...

Correct me if I'm wrong @soareschen, but if we had an impl From<SourceErrorType> for Error for each kind of error we want to map, we could simply just use ? to return and we could cut out the extra map_err step, right? I did this for some of the code in the tendermint-p2p crate for errors that should be mapped from std::io::Error and it seemed to work quite well.

If so, I'd recommend we go ahead and replace all the map_err calls with ? and implement infallible conversions from their respective error types to each crate's respective Error type.

This would, of course, only work for errors where we don't need additional parameters beyond just the source error.

What's the definition of "done" for this issue?

When all the .map_err calls throughout the codebase (wherever possible) for concrete source error types are replaced with automatic conversions (?).

@thanethomson thanethomson added the enhancement New feature or request label Aug 7, 2021
@soareschen
Copy link
Contributor

Sounds reasonable. This should be possible with newer versions of flex-error, since it now defines the error type in the crate that uses it. It may be tricky to generate the instances from the macros, as there can be multiple sub-errors that have the same error sources. So I think it would be better that we derive the From instances manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants