firebase-admin-python icon indicating copy to clipboard operation
firebase-admin-python copied to clipboard

Update support for type hints

Open ViktorSky opened this issue 7 months ago • 4 comments

This pull request introduces comprehensive type annotations to the firebase_admin codebase using PEP 484 standards. All changes have been validated with Pyright in strict mode to enforce consistency and catch potential type errors early.

Goals

  • Enhance developer experience with precise autocomplete and type hints.
  • Catch type-related bugs upfront via static analysis.

Tools

  • Pyright in strict mode for robust type-checking.
  • Manual review of ambiguous or potentially problematic sections.

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.

ViktorSky avatar Jun 05 '25 22:06 ViktorSky

@jamesdaniels @lahirumaramba Could someone please assign a reviewer to this PR? The previous one was completely ignored, and I closed it because it conflicted with the new Firebase version.

ViktorSky avatar Jun 05 '25 22:06 ViktorSky

This feature requires PR https://github.com/firebase/firebase-admin-python/pull/713 to be approved before linters can take advantage of it.

ViktorSky avatar Jun 08 '25 02:06 ViktorSky

tests/test_db.py:907 creates a firebase_admin.db.Reference instance without client.

In this class, the methods assume that client will never be None.

Can you help me here? Should I add a line that creates an instance when it's None? Or should I add validations in each part where it's referenced?

ViktorSky avatar Jun 10 '25 20:06 ViktorSky

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: https://github.com/firebase/firebase-admin-python/pull/892

lahirumaramba avatar Jun 17 '25 17:06 lahirumaramba

I don't know why the 3.9 test fails, could you check it?

ViktorSky avatar Jun 21 '25 20:06 ViktorSky

I don't know why the 3.9 test fails, could you check it?

You can ignore that specific task queue test. That test has been flaky locally, I'll add a fix for that separately.

jonathanedey avatar Jun 23 '25 14:06 jonathanedey

Hey @ViktorSky, thanks again for working on this. As a heads up we are working towards a v7 major release which could have some conflicts with this PR. The current plan is for this PR to not be included in that major release since this is not currently prioritized on our roadmap. Additionally, because of the size and scope of the changes we will review this PR module by module. Please note that this will take some time as this was not something currently on our roadmap.

jonathanedey avatar Jun 25 '25 15:06 jonathanedey

Should I make a pull request to that branch?

ViktorSky avatar Jun 25 '25 16:06 ViktorSky

Let's do that or edit the base of this PR onto the v7 branch. When the v7 changes are eventually merged to master we'll rebase your PR back to master. PR's against v7 branch will also ignore python 3.7 and 3.8 and have an updated pylint version so we wouldn't need to worry about compatibility for those.

jonathanedey avatar Jun 26 '25 13:06 jonathanedey