edgedb / edgedb-python

The official Python client library for EdgeDB
https://edgedb.com
Apache License 2.0
368 stars 44 forks source link

Pydantic models should always be created by codegen #397

Closed brizzbuzz closed 9 months ago

brizzbuzz commented 1 year ago

Hey, it seems like you guys do an optimization where if a Pydantic model already exists with the same signature for a query, new queries will leverage the existing model. While this makes sense at a bits and bytes level, I think it is confusing from a user perspective. For example, say I have two queries

// Create Author
insert Author {
  name := <str>$name
}
// delete Author
delete Author
filter Author.id = <uuid>$id

When I generate the code with edgedb-py --file, it results in the the following

@dataclasses.dataclass
class CreateAuthorResult(NoPydanticValidation):
    id: uuid.UUID

async def create_author(
    executor: edgedb.AsyncIOExecutor,
    *,
    name: str,
) -> CreateAuthorResult:
    return await executor.query_single(
        """\
        insert Author {
          name := <str>$name
        }\
        """,
        name=name,
    )

async def delete_author_by_id(
    executor: edgedb.AsyncIOExecutor,
    *,
    id: uuid.UUID,
) -> CreateAuthorResult | None:
    return await executor.query_single(
        """\
        delete Author
        filter Author.id = <uuid>$id\
        """,
        id=id,
    )

I think it is not ideal to have the method delete_author_by_id have a return type of CreateAuthorResult | None

Since this is generated code, I would much rather just have two duplicate models, one called CreateAuthorResult and the other called DeleteAuthorResult

fantix commented 1 year ago

Thanks for the report! Yeah this is an annonying problem. We have discussed and are planning for a proper name annotation support (no link for the new design yet, pls see https://github.com/edgedb/edgedb/pull/4325 for now). But this would need server changes (before we have language servers!), so please kindly expect a fix in EdgeDB 3.0.

Alternatively and theoretically, we could also add support for custom name annotations for codegen, design proposals are welcome!

(btw it's actually not a Pydantic model but a basic Python dataclass, it's just optimized and compatible when Pydantic is present)

fantix commented 1 year ago

We have discussed and are planning for a proper name annotation support

Now tracking: https://github.com/edgedb/edgedb/pull/5334

jsimonlane commented 9 months ago

@fantix -- seems that you fixed it here https://github.com/edgedb/edgedb/pull/5334.

Close this issue out ?

fantix commented 9 months ago

Right, codegens are now preferring the server names.