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

Bind composite errors #1975

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

edodo1337
Copy link

@edodo1337 edodo1337 commented Dec 2, 2024

I have made things!

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Related issues

This pull request fixes the issue of error types not composing in chains by using Union, improving clarity and type safety. However, Any can still appear in some cases. Setting Success and Failure to use _Nothing or NoReturn instead of Any helps with type inference but breaks many existing tests. I’m not an expert in type theory, but personally, I’d find it very useful to see fully composed error types without Any

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

I am not sure about this feature :)

As I told you in the chat.
But, anyway - thanks a lot for working on it!

Do you see any alternatives?

@@ -119,9 +119,9 @@ def bind(
self,
function: Callable[
[_ValueType],
Kind2['Result', _NewValueType, _ErrorType],
Kind2['Result', _NewValueType, _ErrorType | _NewErrorType],
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this approach is that it works with Exception subtypes, but it does not work if you have Result[int, int] and Result[int, str]

How would you compose these two objects? Result[int, int | str] might not make any sense.

Copy link
Author

@edodo1337 edodo1337 Dec 3, 2024

Choose a reason for hiding this comment

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

How would you compose these two objects? Result[int, int | str] might not make any sense.

Thank you for the review! I see your point, but isn’t int | str better than dealing with Any? At least with int | str, we can use pattern matching to handle each case explicitly.

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (82ef3ef) to head (4648079).
Report is 176 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##            master     #1975    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           80        81     +1     
  Lines         2485      2601   +116     
  Branches       437        44   -393     
==========================================
+ Hits          2485      2601   +116     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Ok, I tested this out. Right now I kinda agree that this makes a lot of inference better.

But, in case we are adopting it - we need to make sure that all ResultLike behave the same way. Can you please address this?

…sult, RequiresContextIOResult, RequiresContextResult
@edodo1337
Copy link
Author

edodo1337 commented Dec 6, 2024

@sobolevn Added similar behavior for ResultLike: IOResult, FutureResult, RequiresContextResult, and RequiresContextIOResult as requested. Let me know if I got everything right or missed something.

@sobolevn
Copy link
Member

sobolevn commented Dec 6, 2024

The CI is failing, please take a look.

@edodo1337
Copy link
Author

There was a linting issue, it should be fine now.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Please, don't forget about interfaces.

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

Successfully merging this pull request may close these issues.

2 participants