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

add custom errors compatible with std #18

Merged
merged 3 commits into from
Jan 5, 2025
Merged

add custom errors compatible with std #18

merged 3 commits into from
Jan 5, 2025

Conversation

oroulet
Copy link
Member

@oroulet oroulet commented Jan 2, 2025

to be used for internal code and not abuse
StatusCode errors from protocol

@oroulet oroulet requested a review from einarmo January 2, 2025 17:12
@oroulet oroulet self-assigned this Jan 2, 2025
@oroulet oroulet force-pushed the errors branch 2 times, most recently from c75d961 to b842997 Compare January 2, 2025 17:50
Copy link
Contributor

@svanharmelen svanharmelen left a comment

Choose a reason for hiding this comment

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

Some small suggestions to tidy up things, but also some suggested fixes as the code currently doesn't compile.

 to be used for internal code and not abuse
 StatusCode errors from protocol

Co-authored-by: Sander van Harmelen <[email protected]>
@oroulet
Copy link
Member Author

oroulet commented Jan 4, 2025

Some small suggestions to tidy up things, but also some suggested fixes as the code currently doesn't compile.

thanks. My last commit was obviosuly done far too fast.

But my main question/proposition with that MR is to create that enum for std compatible errors. I was suprised they did not exist in current code base. I thought maybe I was missing something..

@oroulet oroulet requested a review from svanharmelen January 5, 2025 13:57
Copy link
Contributor

@svanharmelen svanharmelen left a comment

Choose a reason for hiding this comment

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

Just two more little typos and then this should be good to go 👍🏻

oroulet and others added 2 commits January 5, 2025 16:14
Co-authored-by: Sander van Harmelen <[email protected]>
Co-authored-by: Sander van Harmelen <[email protected]>
@oroulet
Copy link
Member Author

oroulet commented Jan 5, 2025

@svanharmelen for some reason, you still have some "change request" preventing the merge But I cannot see for what...

@svanharmelen
Copy link
Contributor

svanharmelen commented Jan 5, 2025

GitHub doesn't automatically dismiss a review when changes are requested (even if those changes are implemented). So once your done (which I can see your are) I can/should take another look and approve the changes if all is good now.

Copy link
Contributor

@svanharmelen svanharmelen left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@svanharmelen svanharmelen merged commit 931ffe2 into master Jan 5, 2025
6 checks passed
@svanharmelen svanharmelen deleted the errors branch January 5, 2025 15:28
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.

3 participants