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

Suggestion: What about replacing Document.Mongo.attribute with Document.__attribute__ #27

Open
devtud opened this issue Apr 15, 2020 · 2 comments
Labels
code quality Non functional code improvements
Milestone

Comments

@devtud
Copy link

devtud commented Apr 15, 2020

First of all, congrats for beginning this project! 👍

Here is a suggestion of change that I believe can improve clarity, simplicity and code suggestion (for some IDEs). Plus, it keeps the document class flat and it gets rid of an inner class.

Instead of

class User(Document):
    name: str
    username: str

    class Mongo:
        collection = 'users'
        indexes = [
            IndexModel('username', unique=True)
        ]

have

class User(Document):
    __collection_name__ = 'users'
    __indexes__ = [
        IndexModel('username', unique=True)
    ]

    name: str
    username: str

I know it's not a big deal and anyone could live with any approach, but it's just an idea I thought is worth mentioning.
What do you think, @codello ?

@codello
Copy link
Owner

codello commented Apr 16, 2020

Hey, thanks a lot for the suggestion.

I’m not sure yet which style I prefer. On one hand I kind of like not having a nested class. Especially improved IDE integration is a good point. On the other hand one might have quite a few of those “configuration-like” fields (validation, a cap for a collection, read/write concern, codec options, collation, ...). Defining all those as __special__ fields may be less readable than having a nested class.

Looking at similar projects it seems to me that there is no clear preference. Django is using its Meta class while SQLAlchemy uses __tablename__, __table__ and __table_args__ without a nested class. Ming also uses nested classes. Some other MongoDB ODMs use approaches that are not compatible with Pydantic.

I’m undecided right now which approach is better but I think I’m going to stick with a nested class for now in order to separate actual data fields in a document from fields that apply to the whole collection. I'm gonna keep this issue open for a while until I have a clearer picture which configuration options I actually want to support. Then I'll come back to this for a final decision.

@codello codello added the code quality Non functional code improvements label Apr 16, 2020
@codello codello added this to the Version 1.0 milestone Apr 16, 2020
@ThibaultLemaire
Copy link

I vote for the current implementation with the nested class.

I agree flat is better than nested, but I am not a big fan of the __special__ syntax either. Plus, the Mongo nested class is very similar to Pydantic's Config class, so, for a user already familiar with Pydantic, this feels very consistent and natural.

Here's an example similar to my current use-case:

from enum import Enum

class SomeEnum(Enum):
    VariantOne = "legacy_value_A"
    VariantTwo = "legacy_value_B"
    VariantThree = "legacy_value_C"

class SomeDocument(Document):
    some_field: SomeEnum
    some_other_field: str

    class Mongo:
        collection = "some_collection"

    class Config:
        use_enum_values = True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Non functional code improvements
Projects
None yet
Development

No branches or pull requests

3 participants