Skip to content

Handler Error type on Master #229

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

Closed
aklitzke opened this issue May 21, 2020 · 5 comments
Closed

Handler Error type on Master #229

aklitzke opened this issue May 21, 2020 · 5 comments

Comments

@aklitzke
Copy link

On current master, the handler requires an anyhow::Error as the error type for the handler, as opposed to Box<dyn std::error::Error + Send + Sync + 'static> in previous versions. However, this new type isn't re-exported from lib, requiring consumers of this crate to also import anyhow. Is this going to stick around? Maybe it would make sense to reexport your error type?

@davidbarsky
Copy link
Contributor

No, it should be Box<dyn std::error::Error + Send + Sync + 'static>. This is a bug, sorry about that. I was lazy in some refactoring and forgot to remove anyhow::Error.

@softprops
Copy link
Contributor

I got this. Covered in #231

@davidbarsky
Copy link
Contributor

Sorry about that, @aklitzke! This should be resolved now.

@aklitzke
Copy link
Author

Awesome, thanks!

Not sure what your strategy is around testing, but should there maybe be some testing around this? Surprised you'd be able to make a breaking change to the api like that and not have some tests/examples fail to compile or something

@softprops
Copy link
Contributor

Few thoughts there.

  1. the state of the current code is always going to be in sync with the tests by virtual of ci. If apis change the tests likely will as well

  2. the state of the master branch reflects the current state of development, not a release. Depending on the current state of development comes with the higher risk of changes commit to commit up until a release

  3. the next release will not be a patch release! Expect some changes from what's hosts on crates.io. all good and useful changes (we hope)

  4. I think it's fair to have some tests that capture a contract for the error type but in the case of current master we've opted not to expose a type alias and instead rely on users to Box up a dyn std Error. Internally we use a type alias. Perhaps on thing that could be improved is type tests to capture it's properties so that when it's type does change we know we're sticking with those well known properies

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

No branches or pull requests

3 participants