search

oxidecomputer / third-party-api-clients

A place for keeping all our generated third party API clients.
https://docs.rs/octorust
MIT License
132 stars 55 forks source link

[github] Specialized errors #57

Closed chantra closed 1 year ago

chantra commented 1 year ago

[github] Use specialized error

Most, if not all errors, are generated by, or wrapped by, anyhow!. This causes all errors to look the same from the caller vantage point, preventing the caller from possibly handling them. For instance, when the API is rate limited, returning an error that indicates this is a RateLimiting error, and providing a simple way to know until when we are being rate limited is valuable. At the moment, one needs to extract the message from the anyhow error, do some regex magic to detect if this is being rate limited or not, and extract until when the rate limitation applies.

This change introduces the use of the crate thiserror and ports the github crate to use it. All crates will now return a ClientResult instead of a anyhow::Result. For crates ported to thiserror, Client result is an alias to Result<T, ClientError>, where ClientError is the enum defined using thiserror to populate variants. If a crate is not onboarded in thiserror, ClientResult is a simple alias to anyhow::Result.

chantra commented 1 year ago

please ignore the first commit (7947203), which is what get spit out when I run make github on top of the current main branch.

Actual change is in 4eb403d.

I have only rebuild github for now, but tested this also on other projects by running make. It generates too much noise to be valuable here. Once I have gotten feedback, I will be more than happy to provide a change which would include everything.

https://gist.github.com/chantra/f1b450ce34fec60061048073df3d7195 would be the generated remaining diff.

chantra commented 1 year ago

actually ported all projects to the specialized errors. The interesting commits are the 3 last ones. The first one just rebuilds them all against my setup which generates a bunch of noisy change.

augustuswm commented 1 year ago

Thanks for putting this all together. I'm definitely on board with the premise of returning specific errors instead of an anyhow error. Give me a little to read through this.

chantra commented 1 year ago

side note: I will need to also port github/src/http_cache.rs to specialized errors. This would likely become a GH only error variant.

chantra commented 1 year ago

@augustuswm let me know how that look like. I can update this PR with only the 2 meaningful commits (vs the rebuild commits) and let CI rebuild upon release.

The conflict shown is because #58 change was not rebuilt.

augustuswm commented 1 year ago

Yeah, if you don't mind reducing it down to only the non-generated files, that would probably be cleaner for a commit. The builds do add a lot of noise. main should now be updated with #58 and rebuild clients.

chantra commented 1 year ago

Thanks. I stripped the 2 extra commits. So only relevant code change is left now.

augustuswm commented 1 year ago

One last edit before merging this. We may iterate on the error variants, but overall this makes sense and it would be good to start building off of it.

chantra commented 1 year ago

One last edit before merging this. We may iterate on the error variants, but overall this makes sense and it would be good to start building off of it.

ok, addressed comments in https://github.com/chantra/oxidecomputer-third-party-api-clients/commit/2b0b71fa4a569dae0b7000382c8dba8babcacd6e

anyhow has been removed from the projects' cargo files as part of this.

chantra commented 1 year ago

Thanks @augustuswm Looks like I forgot the httpcache use case. Will fix forward in a couple of hours. Do you know if there is any other features I should test for other clients?

chantra commented 1 year ago

should be fixed in #63 . Sorry for the trouble.

augustuswm commented 1 year ago

Thanks! All good, I'll get that merged.