Use explicit optional type on arguments

[hoefling]

This allows the type checks with mypy to pass in strict mode.

The reason for this is that we run type checks with mypy using the --strict mode which includes no_implicit_optional. This means that calls like e.g.

async def some_wrapper() -> ExecutionResult:
    schema: GraphQLSchema = ...
    query: str = ...
    variables: Optional[Dict[str, Any]] = ...
    return await graphql(schema, query, variable_values=variables)

fill fail the check with an error

src/mod.py:XXX: error: Argument "variable_values" to "graphql" has incompatible type "Optional[Dict[str, Any]]"; expected "Dict[str, Any]"

A possible workaround would be maintaining a private set of type stubs for the graphql package with optional types wrapped into Optional.

If you find the suggestion useful, I can extend this to the whole codebase. Of course, I understand that e.g.

def execute(
    schema: GraphQLSchema,
    document: DocumentNode,
    root_value: Any = None,
    context_value: Any = None,
    variable_values: Optional[Dict[str, Any]] = None,
    operation_name: Optional[str] = None,
    field_resolver: Optional[GraphQLFieldResolver] = None,
    type_resolver: Optional[GraphQLTypeResolver] = None,
    middleware: Optional[Middleware] = None,
    execution_context_class: Optional[Type["ExecutionContext"]] = None,
) -> AwaitableOrValue[ExecutionResult]:

starts to loose readability, but maybe it's time to introduce type aliases for common args; I find the current API great and already useable to be declared stable.

[Cito]

Thanks for reporting. So far I was quite happy that I did not need to add Optional[...] to arguments with Null default value. It's redundant and really noisy and distracting, because of the additonal brackets and the lengthy "Optional" (I wished they had called it Opt or something).

On the other hand, I see how making that change would improve the experience when checking your code using --strict.

In any case we should make the change either everywhere or nowhere. Currently I get 258 errors with strict_optional = True, but these appear for every call as your example shows, so we probably need to change much fewer places.

As you suggested, we can consider using type aliases. One idea would be to use Maybe as alias for Optional since this is also used in GraphQL.js:

T = TypeVar('T')
Maybe = Optional[T]

class ExecutionResult(NamedTuple):
    data: Maybe[Dict[str, Any]]
    errors: Maybe[List[GraphQLError]]

Or, perhaps:

K = TypeVar('K')
V = TypeVar('V')
MaybeDict = Optional[Dict[K, V]]
MaybeList = Optional[List[V]]

class ExecutionResult(NamedTuple):
    data: MaybeDict[str, Any]
    errors: MaybeList[GraphQLError]

We would spare 3 letters and be more similar to GraphQL.js on the one hand, but be less Pythonic on the other hand.

I'm currently still a bit undecided.

[Cito]

The problem with the type alias is that callers would need to import them to use the same signature. So probably impracticable and not a good idea.

[IvanGoncharov]

One idea would be to use Maybe as alias for Optional since this is also used in GraphQL.js:

@Cito Long term plan is to remove Maybe and replace it with explicit union types:
GraphQLNamedType | null or GraphQLNamedType | void depending on the case.
Maybe was just a workaround introduced by DefinetlyTyped contributors to deal with TS missing ?GraphQLNamedType syntax.

[Cito]

@IvanGoncharov Thanks a lot for mentioning this. So it makes much less sense to use a Maybe alias for us. Optional in Python is only a shorthand for the explicit Union[..., None].

[hoefling]

@Cito I have added the Optional types throughout the codebase, so the mypy --no-implicit-optional check passes. Also turned the no_implicit_optional setting on in mypy config.

[Cito]

The Zen of Python starts with "Beautiful is better than ugly." so we should keep it as it is. But then it continues "Explicit is better than implicit." so we should make that change. It's a real conflict of interests. But ok, well let's do it since you already put so much effort into this. We are following the Zen of Python anyway no matter what we do...

[hoefling]

@Cito thank you! I had another idea to shorten the Optional part - what do you think of an import alias? E.g. the example from above would turn to

from typing import Optional as Opt  # or even O?

def execute(
    schema: GraphQLSchema,
    document: DocumentNode,
    root_value: Any = None,
    context_value: Any = None,
    variable_values: Opt[Dict[str, Any]] = None,
    operation_name: Opt[str] = None,
    field_resolver: Opt[GraphQLFieldResolver] = None,
    type_resolver: Opt[GraphQLTypeResolver] = None,
    middleware: Opt[Middleware] = None,
    execution_context_class: Opt[Type["ExecutionContext"]] = None,
) -> AwaitableOrValue[ExecutionResult]:

[Cito]

Yes, I thought about that, but I'm not sure if that would really help, because people will not understand it immediately when viewing a function signature in an IDE and start to wonder whether Opt is something special in GraphQL, and maybe some tooling will not recognize it properly. I think it should have been called Opt or something shorter in typing from the beginning, since it is used so frequently. Like we say Dict instead of Dictionary. Just found that there was already a discussion about this, but unfortunately the case was closed.

[hoefling]

@Cito I think you might be interested in PEP 604 that adds Scala- and TS-like union types. Not yet approved for 3.9, though.

[Cito]

@hoefling Would be nice to have that in 3.9. The | None syntax is way more readable.

Btw, PEP 589 looks also useful for us e.g. for instrospection types, and it's already in 3.8.