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

Make abort->error conversion one way #434

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

mendess
Copy link
Collaborator

@mendess mendess commented Nov 22, 2023

Having DapError convertible into DapAbort means that an Error can be
converted into an abort and an abort converted into an error an infinite
number of times, meaning the type can grow in an unbounded way at
runtime. This is very confusing to reason about when reading the
signature of a method that returns a DapAbort. Now a method that
returns a DapAbort can only return due to a documented protocol error
and not another system related error (such as mis configuration or IO).

This PR removes the impl From<DapError> for DapAbort implementation and fixes
the code accordingly.

@mendess mendess self-assigned this Nov 22, 2023
@mendess mendess force-pushed the mendess/simplify-error-conversions branch from ac494d0 to df1047c Compare November 22, 2023 15:11
@mendess mendess marked this pull request as draft November 22, 2023 15:30
@mendess mendess force-pushed the mendess/simplify-error-conversions branch from df1047c to 4f2f054 Compare November 22, 2023 18:11
Having DapError convertible into DapAbort means that an Error can be
converted into an abort and an abort converted into an error an infinite
number of times, meaning the type can grow in an unbounded way at
runtime. This is very confusing to reason about when reading the
signature of a method that returns a `DapAbort`. Now a method that
returns a `DapAbort` can only return due to a documented protocol error
and not another system related error (such as mis configuration or IO).
@mendess mendess force-pushed the mendess/simplify-error-conversions branch from 4f2f054 to 8fd75a3 Compare November 22, 2023 18:35
@mendess mendess marked this pull request as ready for review November 22, 2023 19:02
Copy link
Contributor

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea 👍. It doesn't look to me like this changes the HTTP responses, and it most definitely simplifies the code.

@mendess mendess merged commit 21bc5c6 into main Nov 23, 2023
4 checks passed
@mendess mendess deleted the mendess/simplify-error-conversions branch November 23, 2023 14:49
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.

2 participants