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

Adding ADR on UniffiTag handling #1932

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bendk
Copy link
Contributor

@bendk bendk commented Jan 6, 2024

We've discussed this is various issues, most recently #1865. I think it's time that we have an ADR and discuss the question directly and by itself.

There's a lot more complex cases, for example structs/enums that contain fields with types from another crate. I don't think that affects the decision, but please tell me if I'm missing anything here.

@bendk bendk requested a review from a team as a code owner January 6, 2024 00:43
@bendk bendk requested review from skhamis, jplatte and mhammond and removed request for a team January 6, 2024 00:43

- Good, because it's more explicit.
- Good, because it's easier to explain.
Telling users "you always have to add `uniffi::use_*!`" to use a type from another crate is easier than saying you sometimes you need it and sometimes you don't.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This factor seems the most important to me. I don't want to have to go into the details about the orphan rule, remote types, etc. in order to explain to users if they need uniffi::use_record! or not.

@bendk bendk force-pushed the adr-uniffi-tagging branch from 8f616b3 to 6d02cd1 Compare January 12, 2024 21:05
@bendk
Copy link
Contributor Author

bendk commented Jan 12, 2024

Pushed an edit to this that:

  • Uses Uuid and custom type handling as the example. I think this is closer to a real-world scenario.
  • Added an option for removing the tags altogether.

@jplatte
Copy link
Collaborator

jplatte commented Jan 27, 2024

I still think that Option 1 would be extremely annoying for larger projects consisting of multiple crates that don't need much or any bindings of independent external libraries.

Option 3 actually seems very interesting to me but involves a lot of extra work for reducing the boilerplate so I don't know how realistic it is short-term. If somebody wants to look more into boilerplate reduction for that option, I think #[serde(remote = "...")] could be an interesting case study.

Overall, especially because I don't have that much time to put into this, I (still) strongly prefer option 2.

@bendk bendk force-pushed the adr-uniffi-tagging branch from 4b07693 to 1f7030e Compare January 29, 2024 15:24
@skhamis skhamis removed their request for review February 9, 2024 20:59
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