Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Dict function type arguments to Mapping #3121

Closed
rexledesma opened this issue Mar 11, 2024 · 3 comments · Fixed by #3122
Closed

Refactor Dict function type arguments to Mapping #3121

rexledesma opened this issue Mar 11, 2024 · 3 comments · Fixed by #3122

Comments

@rexledesma
Copy link
Contributor

rexledesma commented Mar 11, 2024

When writing sqlglot code against pyright, I encounter errors in my local IDE. The argument types for lineage (and probably others) are too narrow w.r.t. Dict/Mapping types.

From https://docs.python.org/3/library/typing.html#typing.Dict:

Note that to annotate arguments, it is preferred to use an abstract collection type such as Mapping rather than to use dict or typing.Dict.

Fully reproducible code snippet

from typing import Dict

from sqlglot import exp, parse_one
from sqlglot.lineage import lineage


sqlglot_sources: Dict[str, exp.Query] = {}

lineage(column="y", sql=parse_one("SELECT y from x"), sources=sqlglot_sources)

Results in the following error:

Argument of type "dict[str, Query]" cannot be assigned to parameter "sources" of type "Dict[str, str | Query] | None" in function "lineage"
  Type "dict[str, Query]" cannot be assigned to type "Dict[str, str | Query] | None"
    "dict[str, Query]" is incompatible with "Dict[str, str | Query]"
      Type parameter "_VT@dict" is invariant, but "Query" is not the same as "str | Query"
      Consider switching from "dict" to "Mapping" which is covariant in the value type
    "dict[str, Query]" is incompatible with "None"Pylance[reportArgumentType](https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportArgumentType)
@georgesittas
Copy link
Collaborator

If I understand this correctly, this change in the lineage prototype should suffice, right?

-    sources: t.Optional[t.Dict[str, str | exp.Query]] = None,
+    sources: t.Optional[t.Mapping[str, str | exp.Query]] = None,

@rexledesma
Copy link
Contributor Author

I believe so. But I'm surprised there isn't an warning/error thrown by the mypy CI check. I would want the fix to incorporate that. The type signature is an easy change, but is prone to change in the future if not enforced.

FWIW, I'm using typeCheckingMode: "basic" in pyright, so I don't have any fancy configuration: https://microsoft.github.io/pyright/#/configuration?id=main-configuration-options

@georgesittas
Copy link
Collaborator

georgesittas commented Mar 11, 2024

I believe so. But I'm surprised there isn't an warning/error thrown by the mypy CI check. I would want the fix to incorporate that. The type signature is an easy change, but is prone to change in the future if not enforced.

FWIW, I'm using typeCheckingMode: "basic" in pyright, so I don't have any fancy configuration: https://microsoft.github.io/pyright/#/configuration?id=main-configuration-options

mypy does complain if I run it against your example, we just probably didn't have a use case where the dictionary was actually restricted to only Query values so the issue wasn't noticed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants