feat: Add endpoint to fetch GitHub repository contributors #4013
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4013 +/- ##
=======================================
Coverage 96.28% 96.29%
=======================================
Files 1143 1143
Lines 36992 37072 +80
=======================================
+ Hits 35618 35698 +80
Misses 1374 1374 ☔ View full report in Codecov by Sentry. |
…invalid query params
novakzaballa
requested review from
a team and
khvn26
and removed request for
a team
| class PaginatedQueryParams(ABC): | ||
| page: int = field(default=1, init=False) | ||
| page_size: int = field(default=100, init=False) |
There was a problem hiding this comment.
- Why derive from
ABC? - Why
init=False? What if we want to customise pagination?
There was a problem hiding this comment.
- Because I forgot we need to use it for a request and thought we would only need it as a base class for others. Now I changed it to a regular class and it's in use by the get repos endpoint.
- Because derivating without either
init=falseorkw_onlygives an error. I now changed to usekw_onlyto have a better result.
Thanks for cathing this out!
|
@khvn26 I solved your question, but that implied making some adjustments and improvements. Can you please re-review? |
| # Then | ||
| assert response.status_code == status.HTTP_400_BAD_REQUEST | ||
| response_json = response.json() | ||
| if is_invalid_page: |
There was a problem hiding this comment.
An if inside a test is a code smell. I think it's better to have a parametrised detail or response_json argument instead.
There was a problem hiding this comment.
Right, thanks! Done.
| # Then | ||
| assert response.status_code == status.HTTP_400_BAD_REQUEST | ||
| response_json = response.json() | ||
| if is_invalid_page: |
There was a problem hiding this comment.
Same here; consider parametrising detail or response_json argument instead.
| response: requests.Response, | ||
| total_count: int | None = None, | ||
| incomplete_results: bool | None = None, | ||
| ) -> dict[str, Any]: |
There was a problem hiding this comment.
nit: It's not critical, but, since we fully control the creation of this dict, we could add a TypedDict return type to this function that would help introspection.
There was a problem hiding this comment.
Maybe Dataclass or would it be too much? Let's please discuss this. Merging as is FTM.
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingdocs/if required so people know about the feature!Changes
Fetch GitHub repository contributors endpoint added.
Some Data Classes were optimized
How did you test this code?
Unit tests added to cover the new code