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

Throw exceptions instead of the ugly Result monad #23

Merged
merged 5 commits into from
May 14, 2021

Conversation

christopher-dG
Copy link
Member

@christopher-dG christopher-dG commented May 12, 2021

Closes #12, ref #22

I cherry-picked c6bbc7b and it seems to have not bitrotted. In case it's not obvious, this is totally breaking (but consistently so, so updates shouldn't be very difficult).

The gist is: instead of receiving a Result{T} that contains a T (repo, user, PR, etc.), an HTTP response, and exception info, we get back a Tuple{T, Response}. If an exception was thrown in the request, it's thrown to the user too, they can catch it like any other error-handling situation. There's HTTPError is anything that was thrown from HTTP.request, PostProcessorError which generally comes from parsing, and RateLimitedError, which is not new.

Code changes might look like:

# Let's be real, no one actually checks for errors
gh = GitHubAPI(ENV["GITHUB_TOKEN"])

# Before:
resp = get_user(gh)
me = GitForge.value(resp)

# After:
me, _ = get_user(gh)
# Okay, maybe some people do check for errors and such

# Before:
resp = get_user(gh)
if GitForge.exception(resp) !== nothing
    @warn "ahhhh!!!" ex
    return
end
@info "Not rate limited yet" remaining=HTTP.header(GitForge.response(resp), "X-Ratelimit-Remaining")
me = GitForge.value(resp)

# After:
try
    me, resp = get_user(gh)
    @info "Not rate limited yet" remaining=HTTP.header(resp, "X-Ratelimit-Remaining")
catch e
    @warn "ahhhh!!!" ex=(e, catch_backtrace())
end

IMO it just makes a lot more intuitive sense.

TODO: need to update CI to GHA, update Documenter setup and manifest, bump minor version.

@oxinabox
Copy link
Member

I think think this makes more sense.
Use the idioms of the language we are using.
Sometimes there are good reasons for varying this.
But standard REST/CRUD isn't one of them.

How much does this break downstream, e.g. registrstor?
I guess a lot.
OTOH project.toml compat means it can stay on old version forever

@christopher-dG
Copy link
Member Author

OTOH project.toml compat means it can stay on old version forever

Yeah, I think we can just rely on compat here.

@christopher-dG christopher-dG mentioned this pull request May 14, 2021
@christopher-dG christopher-dG merged commit 7066b01 into master May 14, 2021
@christopher-dG christopher-dG deleted the cdg/exceptions branch May 14, 2021 17:20
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

Successfully merging this pull request may close these issues.

Throw custom exceptions instead of returning Result
2 participants