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

added the type 'Optional' to all optional parameters for async functions #855

Closed
wants to merge 1 commit into from

Conversation

dcgudeman
Copy link

@dcgudeman dcgudeman commented Feb 21, 2024

This should fix the typing errors for async functions

Fixes #854

@daniel-sanche

@dcgudeman dcgudeman requested review from a team as code owners February 21, 2024 23:13
Copy link

google-cla bot commented Feb 21, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/python-firestore API. labels Feb 21, 2024
@dcgudeman
Copy link
Author

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

I signed the CLA

@dcgudeman dcgudeman force-pushed the main branch 2 times, most recently from 145172e to 9dd0cdb Compare February 22, 2024 01:43
@daniel-sanche
Copy link
Contributor

daniel-sanche commented Feb 22, 2024

Hey David, thanks for putting this together.

At the root of the typing issues in this library is the fact that the mypy type checker is currently mis-configured, so none of the type hints are verified, and a lot of them are wrong. I don't want to make changes to the type hints without getting the verification check working, to make sure we're making the right changes. But turning it all on at once would force a giant change across the entire codebase. So I'm thinking we'll have to explicitly opt-in individual files as we touch different parts of the code, and get the types fixed over time.

TL;DR: Do you think you can change the mypy session in the noxfile to this to validate the types of the files you changed?:

@nox.session(python=DEFAULT_PYTHON_VERSION)
def mypy(session):
    """Verify type hints are mypy compatible."""
    session.install("-e", ".")
    session.install("mypy", "types-setuptools", "types-protobuf")
    # TODO: type hints in many files are currently broken. Opt-in more files overtime, until we reach full coverage
    session.run("mypy", "--follow-imports=skip", "--ignore-missing-imports",
        "google/cloud/firestore_v1/async_aggregation.py",
        "google/cloud/firestore_v1/async_document.py",
        "google/cloud/firestore_v1/async_batch.py",
        "google/cloud/firestore_v1/async_client.py",
        "google/cloud/firestore_v1/async_collection.py",
        "google/cloud/firestore_v1/async_query.py",
    )u

It looks like with this mypy check running, there are 9 errors that still need to be addressed. If you're willing to take those on and get these files passing, that would be great! But if it looks like too much work, let me know and I can take a look

Thanks

@daniel-sanche daniel-sanche mentioned this pull request Apr 1, 2024
4 tasks
@daniel-sanche
Copy link
Contributor

closing this in favor of #984

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/python-firestore API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typing for optional function parameters are incorrect
2 participants