Feat/type checker - #1637
Conversation
Naramsim
left a comment
Naramsim
left a comment
There was a problem hiding this comment.
Hi Fallen! Thanks for the big and important PR! I reviewed some files and asked some questions.
Note: I didn't review nor take a glimpse at openapi.yml, tests.py, serializers.py, models.py, api.py yet.
There was a problem hiding this comment.
Sorry maybe I explained myself poorly. Can we leave the existing migrations as they are (except for the sorting of imports) and have a single new 0034 migration?
There was a problem hiding this comment.
oh ok my bad will fix it tonight
| return super().retrieve(request, pk) | ||
|
|
||
| pass | ||
| def retrieve(self, request: Request, *args: Any, pk: str | int | None = None, **kwargs: Any) -> Response: |
There was a problem hiding this comment.
Hi, just a curiosity: is the order important here?
self, request: Request, *args: Any, pk: str,
self, request: Request, pk: str, *args: Any,
are the same? Because we have named parameters?
There was a problem hiding this comment.
not the same, order is important that keyword arguments should follow positional parameters.
in case 1 pk is explicitly a keyword argument
in 2nd its positional u can pass either as position or as kwarg
| ) | ||
| class AbilityResource(PokeapiCommonViewset): | ||
| queryset = Ability.objects.all() | ||
| queryset = Ability.objects.select_related("generation") |
There was a problem hiding this comment.
Why do we filter for generation?
There was a problem hiding this comment.
Oh, is this for the N+1 problem?
There was a problem hiding this comment.
not filtering just prefetching/selecting you will notice a similar patter in serializer methods too where we were fetching other models in a for loop over a select_related for example a move might have bunch of version groups fetching the version group in the for loop via its model/serializer makes independent django queries for all related groups instead it can be fetched in one query just by using select_related or pre fetching
| try: | ||
| git_hash = subprocess.check_output(["git", "rev-parse", "HEAD"], stderr=subprocess.DEVNULL).decode().strip() | ||
| except Exception: | ||
| except (subprocess.CalledProcessError, OSError, ValueError): |
There was a problem hiding this comment.
Do we need to be so precise here? catching Exception is bad?
Naramsim
left a comment
Naramsim
left a comment
There was a problem hiding this comment.
Great, I left some comments/questions. The remaining file that I need to review is serializers.py
2 participants
Change description
I have written a detailed write up of the changes and decisions made for this pr in the gist u can find here and supporting details, please do give it a read summarizes all the changes nicely
https://gist.github.com/FallenDeity/aa36487a214ea4c40809949a6ccffe7b
here is a tldr for a quick overview
TLDR
This PR introduces critical performance optimizations, type safety, and linting standards to the codebase:
performance boosts across the board with serializer and django orm query optimizations with the ditto process completing in 8mins compared to 26~ mins in circleci currently more benchmarks in the gist and optimizations done https://app.circleci.com/pipelines/github/PokeAPI/pokeapi/2617/workflows/729700bf-568b-4cf7-8566-0d4399c07ed9/jobs/7002
verified that new payloads by server are 100% same as before via a local script
Given there is some logical changes i would encourage checking it out locally perhaps running it too for better reviewing, I have tested the pr extensively and there shouldnt be any issues but feel free to request changes, the test and build process havent been modified only logical changes are to serializer.py and tests are running fine so this dosent introduce any regressions
AI coding assistance disclosure
Used some AI for analysis of serializers, some more exploration about django serializers and scripting/visualization to facilitate testing and diff across dumps
Contributor check list