Make ArangoClient generic to support typing when a custom serialiser is used about python-arango HOT 9 OPEN

Hi @marcuslimdw,

This is not at all a bad idea. However, if that relies on something which works only on Python 3.12, it's going to heavily reduce the number of Python versions we can support. Currently, we go all the way from Python 3.8 to Python 3.11. So, if a feature requires features that are available only since Python 3.12, we unfortunately cannot adopt it.

Maybe I've misunderstood the limitation to Python 3.12, in which case let me know. I've labeled this as an enhancement so we can keep it around.

from python-arango.

@apetenchea the "inline type parameter" syntax (that's how I think of it) is only available in 3.12, so to support >=3.8, we can use the older syntax instead.

The above example might look like this in 3.8:

T = TypeVar("T")

class Serializer(Protocol, Generic[T]):

    def __call__(self, value: T) -> Json:
        pass


class ArangoClient(Generic[T]):

    def __init__(self, serializer: Serializer[T], *args, **kwargs):
        pass

    def db(self, *args, **kwargs) -> Database[T]:
        pass


class StandardDatabase(Generic[T]):

    def collection(self, *args, **kwargs) -> Collection[T]:
        pass

    
class StandardCollection(Generic[T]):

    def insert(self, document: T, *args, **kwargs):  # no longer Json
        pass

from python-arango.

I agree that having the Json type hint seems overly restrictive. As you pointed out, one can implement a custom serialiser, which might work with completely different data from Json (serialise from bytes, for example).

However, I have thought about it carefully and I am not convinced that going full generic is exactly what we need right now. The reason for having Json in there is not to enforce (the Python ignores type hinting), but to give an idea on how the method is supposed to be used. So while going fully generic would indeed silence potential false-positive type-related warnings, it would render the whole type-hinting thing useless. There are other ways to work around such false positives, apart from modifying the driver.

I'm not adamant against it, and I can see why would someone opt for your proposed solution, but I'm wondering if there's other benefits you see that this potential change could bring to the driver.

from python-arango.

Perhaps it might help if I explained my use case.

At work, we use ArangoDB for storage, but our data models are defined with Pydantic. We've created a custom serialiser that allows us to pass Pydantic models directly to, for example, aql.execute, or insert_many.

We also use Pyright for type checking, which necessarily fails because these methods expect builtin Python objects, but get Pydantic models. To put it another way, we're widening the types that the AQL/Collection methods can take at runtime, but the compile-time types stay the same, causing type checking failure. To me, this is exactly what generics are meant for.

An example that doesn't use Pydantic:

import json
from datetime import datetime
from datetime import timezone
from typing import Any

from arango.client import ArangoClient

# define a custom serialiser that can also handle datetimes
def serialiser(data: Any) -> str:
    if isinstance(data, datetime):
        return data.isoformat()

    return json.dumps(data)


ArangoClient(serializer=serialiser).db().aql.execute(
    "INSERT @document INTO @@collection", bind_vars={"document": {"timestamp": datetime.now(tz=timezone.utc)}, "@collection": "my_collection"}
)

This works at runtime, but fails type checking:

 error: Argument of type "dict[str, dict[str, datetime]]" cannot be assigned to parameter "bind_vars" of type "MutableMapping[str, DataTypes] | None" in function "execute"
    Type "dict[str, dict[str, datetime]]" cannot be assigned to type "MutableMapping[str, DataTypes] | None"
      "dict[str, dict[str, datetime]]" is incompatible with "MutableMapping[str, DataTypes]"
        Type parameter "_VT@MutableMapping" is invariant, but "dict[str, datetime]" is not the same as "DataTypes"
      "dict[str, dict[str, datetime]]" is incompatible with "None" (reportArgumentType)

There are other ways to work around such false positives, apart from modifying the driver.

Could you please suggest an alternative? I'd be happy to hear any possible solution to my problem.

from python-arango.