Skip to content

feat: add type hints to pokemon_v2/api.py#1521

Open
jarrensj wants to merge 3 commits into
PokeAPI:masterfrom
jarrensj:feat/type-hints-pokemon-v2
Open

feat: add type hints to pokemon_v2/api.py#1521
jarrensj wants to merge 3 commits into
PokeAPI:masterfrom
jarrensj:feat/type-hints-pokemon-v2

Conversation

@jarrensj

@jarrensj jarrensj commented May 12, 2026

Copy link
Copy Markdown

Summary

Adds Python type annotations to pokemon_v2/api.py. Pure annotation change — no runtime behavior modified.

What's annotated:

  • The 6 method signatures: ListOrDetailSerialRelation.get_serializer_class, NameOrIdRetrieval.get_queryset / get_object, PokeapiCommonViewset.retrieve, PokemonEncounterView.get, PokeapiMetaView.get
  • Class attributes: list_serializer_class, idPattern, namePattern

Types used: canonical DRF / Django imports — Request, Response, BaseSerializer, QuerySet, Model, re.Pattern[str], and PEP 604 unions (X | None).

Motivation

The pokemon_v2 module currently has no type annotations anywhere (api.py / models.py / serializers.py). This PR is the first step toward fixing that. Annotations give readers and IDEs a clear contract on method inputs/outputs without changing runtime behavior, and lay groundwork for adding mypy to CI down the road if maintainers want stricter checks.

Scope and limitations

  • api.py only. pokemon_v2/models.py has no method definitions (only Django field declarations), so there is nothing to annotate there without pulling in django-stubs, which is a much larger change.
  • serializers.py is intentionally out of scope for this PR.
  • No mypy enforcement added; these annotations are documentation-grade rather than type-checked. Happy to follow up with a CI hook if of interest.

Test plan

  • make format-check passes locally (black 25.11.0)
  • make test (verified green on CircleCI)
  • make openapi-generate (verified green on CircleCI)

@jarrensj
jarrensj marked this pull request as ready for review May 12, 2026 07:54
@jemarq04

Copy link
Copy Markdown
Member

Before this moves forward, please read our Contributing guide and see that fully automated AI PRs will not be accepted. Seeing that you overwrote the PR template that would acknowledge that with a large AI generated description, and some other details in the code changes, I think you missed that.

Before we continue with this, I'd like you to note explicitly how you used AI for these changes.

@jarrensj

Copy link
Copy Markdown
Author

Before this moves forward, please read our Contributing guide and see that fully automated AI PRs will not be accepted. Seeing that you overwrote the PR template that would acknowledge that with a large AI generated description, and some other details in the code changes, I think you missed that.

Before we continue with this, I'd like you to note explicitly how you used AI for these changes.

Hi, it's not a fully automated ai pr but I did leverage AI to write the pr description and I think did a great job outlining the change clearly. Let me know if that's okay! It did pass all the checks and tests, too.

@jemarq04

Copy link
Copy Markdown
Member

Ok, thank you for the clarification.

Generally type annotations are also used for variable definitions in addition to the signatures that you updated here, which would be a large undertaking for this project. I don't think it's a bad idea, it would just take some time to update everything. From what I can tell, the annotations you've added look correct.

@phalt @Naramsim What do you two think about this PR (and ones that would follow)? This pertains more to long-term maintenance so I think this would be your call.

As a side note, there was a recent push in #1391 to move to the astral suite (ruff, uv, etc.) and we'd likely move to ty instead of mypy if we went this route.

@phalt

phalt commented May 13, 2026

Copy link
Copy Markdown
Member

Hi, it's not a fully automated ai pr but I did leverage AI to write the pr description and I think did a great job outlining the change clearly. Let me know if that's okay! It did pass all the checks and tests, too.

Yeah that's fine but the point of the template is to include a note on how AI was used. Erasing the template for your own content was why we asked 😄

@phalt @Naramsim What do you two think about this PR (and ones that would follow)? This pertains more to long-term maintenance so I think this would be your call.

I think we should start doing more of this to revive the codebase a bit and bring it into modern Python. I started this already but I have struggled to find the time dedicated to getting my changes shipped.

It probably needs breaking up into tiny tiny changes and working on iteratively.

As a side note, there was a recent push in #1391 to move to the astral suite (ruff, uv, etc.) and we'd likely move to ty instead of mypy if we went this route.

Yes we should adopt ty along with ruff and uv.

Currently though this code isn't tested against any type linter so merging is basically pointless until we can verify it passes a type checker.

@jemarq04

Copy link
Copy Markdown
Member

Currently though this code isn't tested against any type linter so merging is basically pointless until we can verify it passes a type checker.

I agree - we probably shouldn't merge this until we've made the steps toward adopting the astral suite officially and ironing out any bugs that we can find once that's ready.

I think once we've adopted ruff and uv, we could add a target to the makefile to run ty on the project. Of course this would definitely fail, so we shouldn't incorporate that into any formal requirements for PRs and such, but it can give us an idea of what we'd need to reach that goal. Some initial testing with mypy and ty on api.py (with these changes) are failing for me, so this PR may need some additional checks too before it would move forward.

@phalt

phalt commented May 14, 2026

Copy link
Copy Markdown
Member

We might want to take my original PR which does everything and split it into smaller progressive chunks that we merge in.

@FallenDeity

FallenDeity commented May 16, 2026

Copy link
Copy Markdown
Contributor

just another addition on this for future we can use | for None union for making it optional since we are using python 3.13 for builds

regarding @phalt's pr it should be a pretty good starting point we can break it down to 3 parts with uv for dep mgmt and updating all the build commands + pipelines and then progressively enabling ruff and ty on the codebase, in subsequent prs to avoid touching a huge number of files in one go

@Naramsim

Copy link
Copy Markdown
Member

Hi! @jarrensj and @FallenDeity could you take a look at these PR again?

@jarrensj
jarrensj force-pushed the feat/type-hints-pokemon-v2 branch from 8694b51 to 138a785 Compare July 25, 2026 08:17
@jarrensj

jarrensj commented Jul 25, 2026

Copy link
Copy Markdown
Author

Hi! @jarrensj and @FallenDeity could you take a look at these PR again?

Thanks for the ping!

I've updated the PR:

  • rebased onto the latest master branch and resolved conflicts
  • switched to X | None union syntax as per @FallenDeity's suggestion and dropped the typing.Optional import
  • On the type checker concern: now that the uv is merged feat: Migrate package manager to uv #1553 , I ran the check against both master and this branch and report the same diagnostics so no new regressions compared to the current master (baseline)

@FallenDeity

FallenDeity commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

some imports are being added just for typehints imports like those tend to have performance overhead I would suggest to import those under typing.TYPE_CHECKING https://docs.python.org/3/library/typing.html#typing.TYPE_CHECKING so it doesn't affect runtime and the type checker is happy too

on another note ruff is not added to project as a dependency yet currently there is a pr which adds ruff and migrates from black #1602 and my plan was to take this in stages with formatting first followed by type checking in the later prs

also I am not convinced we should use ty given it still has only partial or some unsupported features in comparison to something like pyright or other type checkers check the conformance report by the python org here https://github.com/python/typing/blob/main/conformance/results/results.html so I think this pr can be held of for a bit till we add the type checking process to the repo

@jemarq04

jemarq04 commented Jul 25, 2026

Copy link
Copy Markdown
Member

I think we were waiting until we adopted the astral suite fully (including ruff) before we moved forward with the type hints and so on. Either we move to ty or use another modern type checker like FallenDeity is saying, but either way we should hold on until we've changed some of the dependencies on the project and such.

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 this pull request may close these issues.

5 participants