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

Use Either for error handling #115

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Use Either for error handling #115

merged 4 commits into from
Jan 31, 2024

Conversation

sstone
Copy link
Member

@sstone sstone commented Jan 31, 2024

We already export an Either type, which can be reused by clients of this library.

We already export an `Either` type, which can be reused by clients of this library.
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

I guess the goal is to delete the corresponding class from lightning-kmp when we update it to use this version of bitcoin-kmp, right? In that case we should also import the Try implementation, this way we remove it as well from lightning-kmp, otherwise it would be weird to keep one in lightning-kmp and the other in bitcoin-kmp?

@sstone
Copy link
Member Author

sstone commented Jan 31, 2024

I guess the goal is to delete the corresponding class from lightning-kmp when we update it to use this version of bitcoin-kmp, right? In that case we should also import the Try implementation, this way we remove it as well from lightning-kmp, otherwise it would be weird to keep one in lightning-kmp and the other in bitcoin-kmp?

Done in 7af325c, though I believe that we may not need it: API methods in lightning-kmp should use Either, and internal methods could just use kotlin's built-in Result type

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM, I believe we can revert the version change and merge this 👍

@t-bast
Copy link
Member

t-bast commented Jan 31, 2024

Done in 7af325c, though I believe that we may not need it: API methods in lightning-kmp should use Either, and internal methods could just use kotlin's built-in Result type

I agree with that, but inside lightning-kmp we do use Try in a few places, and it will be cleaner to use the one exported by bitcoin-kmp. Then maybe we can stop using Try altogether from ligthning-kmp if it doesn't make sense, but we can do that in a next step, no need to do it right now!

@sstone
Copy link
Member Author

sstone commented Jan 31, 2024

LGTM, I believe we can revert the version change and merge this 👍

done in e3ba135

@sstone sstone merged commit 7d9f499 into master Jan 31, 2024
@sstone sstone deleted the snapshot/use-either branch January 31, 2024 16:12
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