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

Support models which have generics. #37

Closed
wants to merge 2 commits into from

Conversation

magiclen
Copy link
Contributor

To support models like the following struct,

#[derive(Debug, Serialize, Deserialize, Model)]
struct Person<'a> {
    #[serde(rename = "_id", skip_serializing_if = "Option::is_none")]
    id: Option<ObjectId>,
    name: Cow<'a, str>
}

Copy link
Owner

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Looks like the compile tests are failing. We'll need to make sure those are squared away before we merge this. There is information in the contributing doc here on how to run the compile tests locally on your system.

@magiclen
Copy link
Contributor Author

The reason for the failure is that I changed the lifetime 'a to '_de because 'a is too often to be used by developers. I think it is not appropriate to be an internal-generated lifetime. Anyway, the test case has been fixed.

@thedodd
Copy link
Owner

thedodd commented Jul 24, 2019

@magiclen so an additional request for you here. It would be great to add an additional compile test which covers the exact example that you have in the description for this PR. That way we know the compiler is accepting the original model patterns, in addition to the new model pattern you've introduced here.

Should be as simple as defining the new model type here: https://github.com/thedodd/wither/blob/master/wither_tests/tests/modelgen.rs#L80 along with a minimal number of asserts (most will be covered by previous tests). That will ensure that the model's code-gen is working as needed and that such models will be usable.

@thedodd
Copy link
Owner

thedodd commented Sep 10, 2019

@magiclen this is a copy/paste from #35:

I've been leaning towards using the DeserializeOwned bound on these models, and given the nature of a data model, I'm inclined to say that it should not be generic. Instead ... perhaps if you need a generic model, instead of using generics, you could use a bson field type. That would allow you to use the same model, and then just deserialize the field type as needed. You could even use an enum type with serde's internally tagged serialization model, which would remove the need for generic types.

As far as generic lifetime bounds ... well, when taking data directly out of the database, it doesn't make much sense. I am inclined to keep generic types & lifetimes out of the models in order to keep them simple and direct.

Thoughts?

@thedodd
Copy link
Owner

thedodd commented Dec 19, 2019

Closing in favor of using non-generic enum patterns which work quite nicely with this system already.

@thedodd thedodd closed this Dec 19, 2019
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