Update support for type hints #886
Conversation
|
@jamesdaniels @lahirumaramba |
|
This feature requires PR #713 to be approved before linters can take advantage of it. |
|
In this class, the methods assume that client will never be None. Can you help me here? |
There was a problem hiding this comment.
Thank you for your contribution! We understand that this is an important addition to the the codebase, but as this was not something we had prioritized in the current roadmap the review process will take some time to complete.
I have added a comment to start the process. Do you have any objections to change the import statements to specific items over importing the entire typings module?
firebase_admin/__init__.py
Outdated
| @@ -17,23 +17,31 @@ | |||
| import json | |||
| import os | |||
| import threading | |||
| import typing | |||
There was a problem hiding this comment.
Is there a reason to import the entire module here over importing specific items as needed?
from typing import Any, Callable, List, Optional, Tuple, Coroutine
There was a problem hiding this comment.
I handled it by considering the use of a star import and not defining __all__.
When this type of import is used, it brings in all symbols from the module that do not start with _, including those that were imported.
However, if __all__ is defined, only the names listed there will be imported.
This also helps separate what is used for type hints from what is accessed at runtime.
That said, it ultimately depends on the preferences of other contributors whether they favor explicit names during import or during use.
There was a problem hiding this comment.
Let's align with the Python Style guides here and instead of importing the entire typing module, let's import only the specific types we need. This makes the code more readable and avoids cluttering the namespace.
from typing import List, Dict, Union etc.
https://google.github.io/styleguide/pyguide.html#2241-exemptions
https://peps.python.org/pep-0008/#imports
- `firebase_admin._auth_utils._parse_error_body`: AttributeError: 'str' object has no attribute 'get' - `firebase_admin._http_client.HttpClient.headers`: TypeError: 'ABCMeta' object is not subscriptable - `firebase_admin.db.Reference.__init__`: ValueError: Invalid path: "None". Path must be a string. - `firebase_admin.functions.TaskQueue._validate_task_options`: TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union - `firebase_admin.ml.Model._convert_to_millis`: TypeError: can't subtract offset-naive and offset-aware datetimes
|
Note: you probably won't have to worry about 3.8 compatibility issues as we plan to drop 3.8 in the upcoming major release. See: #892 |
This pull request introduces comprehensive type annotations to the
firebase_admincodebase using PEP 484 standards. All changes have been validated with Pyright in strict mode to enforce consistency and catch potential type errors early.Goals
Tools
Configuration Additions
pyrightconfig.json: config file created to declare strict-mode settings.firebase_admin/_typing.py: A new central module that defines all custom type aliases, TypedDicts, and Protocols. By consolidating these shared types in one place, we simplify extension and reuse across the package and avoid circular-import issues.