search

MongoEngine / eve-mongoengine

An Eve extension for MongoEngine ODM support
Other
39 stars 28 forks source link

Add schema for list and embedded fields #6

Closed dmitryax closed 10 years ago

dmitryax commented 10 years ago

Hi! What do you think of adding schema for for fields like EmbeddedDocumentField and ListField into generated eve models schema

hellerstanislav commented 10 years ago

Hello, in the first place: thanks for your interest into eve_mongoengine and for your work!

This PR reminds me something - I think there was some time frame in the history of eve_mongoengine, when EmbeddedDocumentField was transformed into its own schema: see these commits. Here it was introduced: https://github.com/hellerstanislav/eve-mongoengine/commit/aca37fe793c7d365702fd421195f3dfd5ce7e987 and here https://github.com/hellerstanislav/eve-mongoengine/commit/3bf7ef1b9f1fed2d90672c3978fb50bc57de1260 it was deleted for some reason, but I do not remember why...the commit message is also confusing - "Tuned EmbeddedDocumentField, added tests for it." ... piece of crap.. :)

Also, is there any reason why we want to get schema for these fields? I think that the functionality is basically the same, is'nt it? Maybe one reason could be that it has more eve-like error messages. Or can you see some other reason? At this time I think it is fine tuning of eve_mongoengine, I do not have anything against merging this PR (it doesnt hurt any test, all is OK), but I would like to hear your opinion over this.

Cheers, S.

dmitryax commented 10 years ago

Thank you for response.

I wrote a patch for Eve which enables embedding in subdocuments: https://github.com/nicolaiarocci/eve/pull/389 It doesn't work when there is no schema for these fields. And I think other reasons for that could be found.

hellerstanislav commented 10 years ago

Okay, this is pretty good reason to have this feature in eve-mongoengine. Merged.

Thank you!