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

Fix IDs v2 #242

Closed
wants to merge 2 commits into from
Closed

Fix IDs v2 #242

wants to merge 2 commits into from

Conversation

marioortizmanero
Copy link
Collaborator

Description

This attempt tries to implement what i described in #203:

A different approach would be to scratch Id<T> and just use ArtistId and similars directly. What I mean is that we wouldn't have a single type for IDs called Id<T> that implements its logic. We could make ArtistId and similars, and implement the trait the same way clients work. The main component would be a trait that implements all the logic, say Id, and a struct for each type (ArtistId, etc) that implements the trait. With this one could be generic over Id directly, have Box<dyn Id> for runtime usage, and Vec<Box>` for multiple values, which is exactly what we need.

The main problems are:

  1. We can't use type inference to our benefit; Id::from_uri can't work with this solution; ArtistId::from_uri would have to be used explicitly.
  2. We need the trait Id always available when using IDs; we'd have to add it to the prelude. Considering the user needs to import the prelude to use the clients anyway I don't think it's a big deal

So this solution is basically the same as how clients work. We have a big trait that does everything, and we let Rust "clone" the code for each of our implementations. It's also basically the newtype pattern, but with a common trait for all the newtypes.

@marioortizmanero
Copy link
Collaborator Author

Another failure, unfortunately. It's almost impossible to make these traits object-safe without putting its entire implementation in the macro :(

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.

1 participant