Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1512 +/- ##
=======================================
Coverage 95.30% 95.30%
=======================================
Files 43 43
Lines 3198 3198
=======================================
Hits 3048 3048
Misses 150 150 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Should be rebased on #1523 to game the coverage check |
|
|
||
| def get_plans(self) -> PlanResponse: | ||
| return self._request_and_deserialize("/plans", PlanResponse) | ||
| return self._request_and_deserialize("/api/v1/plans", PlanResponse) |
There was a problem hiding this comment.
def _request_and_deserialize(
self,
suffix: str,
target_type: type[T],
data: Mapping[str, Any] | None = None,
method="GET",
get_exception: Callable[[requests.Response], Exception | None] = _exception,
params: Mapping[str, Any] | None = None,
prefix:str="/api/v1"
) -> T:
url = self._config.url.unicode_string().removesuffix("/") + prefix + suffix
Something like this looks more maintainable ?
This endpoint will need the /api/v1 prefix as well for this to work
There was a problem hiding this comment.
It would make this a smaller change but does bake in the assumption that v1 will always be the default. If we move to v2 being the main version in future we still need to make this change or accept that we will forever be passing prefix="/api/v2" in every call.
This endpoint will need the /api/v1 prefix as well for this to work
Why couldn't the client pass prefix="" when it was called?
There was a problem hiding this comment.
Why couldn't the client pass prefix="" when it was called?
We can but I think this should have also move to /api/v1 for consistency
It would make this a smaller change but does bake in the assumption that v1 will always be the default. If we move to v2 being the main version in future we still need to make this change or accept that we will forever be passing prefix="/api/v2" in every call.
for v2 if the above approach the call will look like prefix="api/v2" ,"/endpoint" instead of "/api/v2/endpoint" which is kind off annoying But can't we do something like this
def _request_and_deserialize(endpoint,prefix="api/v1"):
...
def _request_and_deserialize_v2(endpoint,prefix="api/v2",):
_request_and_deserialize(endpoint,prefix)
...
and call _request_and_deserialize_v2 ?
Personally I like the approach I have mention of using a prefix but I'm not fussed about it
No description provided.