Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||
| adapter = HTTPAdapter(max_retries=retry_strategy) | ||
|
|
||
| with requests.Session() as session: | ||
| session.mount("https://", adapter) |
There was a problem hiding this comment.
Might not be reliable at times. E.g. when the developer is using a local proxy.
There was a problem hiding this comment.
also added:
session.mount("http://", adapter)
| total=self.config.requests_options.get("retries"), | ||
| backoff_factor=self.config.requests_options.get("backoff_factor", 0), | ||
| status_forcelist=self.config.requests_options.get("status_forcelist"), |
There was a problem hiding this comment.
The developer might mistakenly believe that retries and backoff_factor etc. are requests options (since they are passed as config.requests_options). This is not entirely true, and might cause confusion.
Alternative:
self.config.requests_retry_options.get("total")
self.config.requests_retry_options.get("backoff_factor")
...
Or even:
self.config.requests_retry_options.total
self.config.requests_retry_options.backoff_factor
...
💡
| except Exception as err: | ||
| raise NetworkProviderError(url, err) | ||
|
|
||
| def _remove_unneeded_options_for_request(self) -> dict[str, Any]: |
There was a problem hiding this comment.
This hack / workaround can be removed if we use the approach with separate retry options.
| def _do_post(self, url: str, payload: Any) -> dict[str, Any]: | ||
| try: | ||
| response = requests.post(url, json=payload, **self.config.requests_options) | ||
| request_options = self._remove_unneeded_options_for_request() |
There was a problem hiding this comment.
Hack / workaround can be reverted etc.
| def _do_get(self, url: str) -> GenericResponse: | ||
| try: | ||
| response = requests.get(url, **self.config.requests_options) | ||
| retry_strategy = Retry( |
| class RequestsRetryOptions: | ||
| retries: int = 3 | ||
| backoff_factor: float = 0.05 | ||
| status_forecelist: list[int] = field(default_factory=lambda: [500, 502, 503, 504]) |
There was a problem hiding this comment.
They seem fine. If there are constants in Python standard library, we can use them.
There was a problem hiding this comment.
replaced with constants
|
|
||
| adapter = HTTPAdapter(max_retries=retry_strategy) | ||
|
|
||
| with requests.Session() as session: |
There was a problem hiding this comment.
Hopefully, no performance overhead with creating a lot of ephemeral Session objects 🤞
We should keep an eye on this matter.
| total=self.config.requests_retry_options.retries, | ||
| backoff_factor=self.config.requests_retry_options.backoff_factor, | ||
| status_forcelist=self.config.requests_retry_options.status_forecelist, | ||
| ) | ||
|
|
||
| adapter = HTTPAdapter(max_retries=retry_strategy) | ||
|
|
||
| with requests.Session() as session: | ||
| session.mount("https://", adapter) | ||
| session.mount("http://", adapter) |
There was a problem hiding this comment.
A little bit of duplication. In the future, maybe we can extract something to shared.py - but only if it really makes sense.
3 participants
No description provided.