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

Kotlin result #728

Merged
merged 5 commits into from
Nov 10, 2024
Merged

Kotlin result #728

merged 5 commits into from
Nov 10, 2024

Conversation

jcrist1
Copy link
Contributor

@jcrist1 jcrist1 commented Nov 9, 2024

Uses kotlin result. An attempt to address #697. I have to admit I'm not a big fan. On the plus side it is more ergonomic, but a big minus is we completely erase the type of the error. Unfortunately as per the discussion in the issue we can't make a generic wrapping class. Instead we would have to change the error type to extend Throwable. Let me know what y'all think of this.

@jcrist1 jcrist1 marked this pull request as ready for review November 9, 2024 07:55
Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

(a) can we get rid of Res?
(b) Can we replace the .ok/.err methods that seem to be globally added (?) with local free functions?

@jcrist1
Copy link
Contributor Author

jcrist1 commented Nov 10, 2024

(a) can we get rid of Res?

Yes and done.

(b) Can we replace the .ok/.err methods that seem to be globally added (?) with local free functions?

I'm not sure I understand. We can move them to always be local to the class, but I don't understand the benefit as it would just duplicate the code accross classes. Could you elaborate on the reasoning? In any case it should be a small change. I've made them internal now, so they shouldn't escape the library.

As for free functions, I'm not familiary with the term. A quick googling tells me it's like a function which doesn't take a "self" param in Rust's terminology. However, a lot of the code flow assumes that the .ok() and .err() will be applied as postfix operations, as such it would require more changes to get them to be free functions, if I'm understanding it correctly.

@Manishearth
Copy link
Contributor

We can move them to always be local to the class,

No I just meant package local. internal is fine.

As for free functions, I'm not familiary with the term. A quick googling tells me it's like a function which doesn't take a "self" param in Rust's terminology.

I was thinking of a free function as in the equivalent of a standalone fn in rust, not on an object. If Kotlin doesn't have that, then perhaps a static method on some shared utility class.

But this is fine too!!

@Manishearth Manishearth merged commit 0650c74 into rust-diplomat:main Nov 10, 2024
20 checks passed
@Manishearth
Copy link
Contributor

cc @emarteca

@emarteca
Copy link
Contributor

emarteca commented Dec 6, 2024

Thanks for doing this!

I just made a PR making the struct and opaque error results Throwable; I think it's worth being able to inspect/use the error on the Kotlin side. #741

Other types are hard to return so that Kotlin can actually use them, since we can't make them Throwable too.
Do we think it might be worth creating an error wrapper type for each primitive? Unfortunately, as you guys noted, it seems the JVM languages won't let us make a generic class Throwable, b/c it's unable to reflect on the type. :(

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