search

databricks / databricks-sdk-py

Databricks SDK for Python (Beta)
https://databricks-sdk-py.readthedocs.io/
Apache License 2.0
343 stars 113 forks source link

[ISSUE] Lots of missing `Optional`s in the type hinting #564

Open judahrand opened 6 months ago

judahrand commented 6 months ago

Description

In lots of places the SDK's type hints look like:

foo: str = None

This typing is incorrect and should be:

foo: str | None = None
PeterJCLaw commented 6 months ago

@judahrand are you aware of mypy's --implicit-optional behaviour? The foo: str = None style is valid when using that option. (I don't know if other checkers have a similar behaviour, though it's likely they do)

I appreciate that not everyone uses that style (and I would personally discourage library authors from doing so as being explicit is usually clearer), however my understanding is that the spelling used in this project shouldn't impact how your project is type-checked. If it does, that may be a bug (e.g: https://github.com/python/mypy/issues/9208) in your type checker and/or you may need to configure handling of implicit optionals if it's intentional that this project uses that style.

mgyucht commented 6 months ago

To be honest, this project doesn't make use of mypy typechecking quite yet. We've been meaning to add support for it, and I think we'd be open to discussion about whether enabling --implicit-optional is preferable or not for this project.

judahrand commented 6 months ago

I think that using explicit Optional is probably preferable given that not using it requires downstream consumers to have to configure their type checker correctly vs just using the defaults.

mgyucht commented 6 months ago

This seems reasonable to me, especially as this seems to be the direction that mypy and the ecosystem are moving towards.

judahrand commented 6 months ago

This seems reasonable to me, especially as this seems to be the direction that mypy and the ecosystem are moving towards.

This might not be the right place to raise this but it would be nice if this could be changed/updated in databricks-connect too. I'd open a PR but unfortunately the source isn't available anywhere other that PyPi.