codello / Motor-ODM

A MongoDB ODM based on Motor and Pydantic.
MIT License
17 stars 5 forks source link

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

Open devtud opened 4 years ago

devtud commented 4 years ago

First of all, congrats for beginning this project! :+1:

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 commented 4 years ago

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.

ThibaultLemaire commented 3 years ago

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