Skip to content

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

Open
jarrensj wants to merge 2 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 2 commits into
PokeAPI:masterfrom
jarrensj:feat/type-hints-pokemon-v2

Conversation

@jarrensj
Copy link
Copy Markdown

@jarrensj jarrensj commented May 12, 2026

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, PokeapiMetaViewset.list
  • Class attributes: list_serializer_class, idPattern, namePattern

Types used: canonical DRF / Django imports — Request, Response, BaseSerializer, QuerySet, Model, re.Pattern[str], Optional[...].

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
Copy link
Copy Markdown
Member

phalt commented May 13, 2026

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.

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.

3 participants