move table_name to model_config

[mplemay]

in order to be more consistent with the pydantic api, I propose that the table_name field should be moved to a model_config TypedDict which extends the base TypedDict.

Let me know if you want me to implement this.

[piercefreeman]

@mplemay Would you propose this is extended to all class arguments, ie both table_name and table_args right now? If so it feels a bit unergonomic for table_args to be squished inside the model_config. When I think of a standard model_config I'm also thinking specifically of serialization/deserialization behavior, which are most of the arguments within the parent model_config. I could get over that bias but I do believe that it needs to be a joint decision for both of them to be defined in the same place.

It would be a pretty light lift to change it architecturally. But Pydantic does provide the ability for subclasses to implement ClassVar for attributes that should be defined on the class level and explicitly excluded from the instance creation syntax. In that sense keeping them as class-vars seems to still be consistent with the pydantic API.

WDYT?

[mplemay]

@piercefreeman I am definitely biased, but I think ModelConfig approach makes sense for the following reasons:

1. It puts all of the tables "options"/"settings' in a place that is familiar to pydantic users (see the pydantic_settings example)
2. It removes values from the table which seemingly look like fields. For example:

class MyTable(TableBase):
    id: int = Field(primary_key=True)
    table_name = "my_table"
    table_scheme = "my_schema"

Whereas the following it is much cleared what is a field and what is not.

class MyTable(TableBase):
    model_config = {"table": "my_table", "schema": "my_schema"}
    
    id: int = Field(primary_key=True)

And when comes to table_args, I am partial to this proposal (#61 (comment)). TLDR: I think it would make sense to do something akin to:

class MyTable(TableBase):
    id: int = Field(primary_key=True)
    age: int

    def _constraints_(self) -> Sequence[...]:
        return [
            CheckConstraint("age_verification", MyTable.age > 21),
        ]

The reason being is, it allows the user to specify all kind of constraints, indexes, etc. with typed fields, functions, etc. and since it is not apart of the model, it am pretty sure it will avoid errors like:

from iceaxe import Field, TableBase


class MyTable(TableBase):
    id: int = Field(primary_key=True)
    thing: str

print(MyTable.model_json_schema())

  File "/Users/matthewlemay/Github/brugge/.venv/lib/python3.12/site-packages/pydantic/json_schema.py", line 2273, in handle_invalid_for_json_schema
    raise PydanticInvalidForJsonSchema(f'Cannot generate a JsonSchema for {error_info}')
pydantic.errors.PydanticInvalidForJsonSchema: Cannot generate a JsonSchema for core_schema.CallableSchema

For further information visit https://errors.pydantic.dev/2.10/u/invalid-for-json-schema

Let me know what your thoughts!

[piercefreeman]

@mplemay I'm coming around to the approach you're laying out here, at least for model_config. I'm still not a huge believer in function-based constraint functions because function execution requires some runtime processing and database definitions should be declarative as they are in Postgres.

Do you want to take a crack at implementing this? Should be reverse-compatible (at least for now) with a possible deprecation flag indicating that we're switching to the new format?

[mplemay]

I'm still not a huge believer in function-based constraint functions because function execution requires some runtime processing and database definitions should be declarative as they are in Postgres.

I hear you on this. One of the key challenges with mapping posgres to python in my opinion is the expectations that come along with balancing the two different approaches. In order to make the library work how a python user would expect (namely with type checking & static validation using linters), I'm not sure there is a way to achieve this with a purely declarative approach that makes sense.

Do you want to take a crack at implementing this? Should be reverse-compatible (at least for now) with a possible deprecation flag indicating that we're switching to the new format?

Sure. I'll start digging into this over the next week or two.