Added LagAwareHealthCheck for MultiDBClient #3737
Conversation
| self, | ||
| path: str, | ||
| *, | ||
| params: Optional[Mapping[str, Union[str, int, float, bool, None, list, tuple]]] = None, |
There was a problem hiding this comment.
The Union should be Optional.
There was a problem hiding this comment.
But within Union we already define None, value could be anything including None
There was a problem hiding this comment.
Optional makes sense if it's either one value or None. But we have a Union of multiple values including None
redis/http/http_client.py
Outdated
| auth_basic: Optional[Tuple[str, str]] = None, # (username, password) | ||
| user_agent: str = DEFAULT_USER_AGENT, | ||
| ) -> None: | ||
| self.base_url = base_url.rstrip("/") + "/" if base_url and not base_url.endswith("/") else base_url |
There was a problem hiding this comment.
Why do you call rstrip to remove the slash - in this case, you have already checked that the url doesn't end with a slash? Maybe here the rstrip + adding a single slash should be used when the url end with a slash - to ensure that it is only one slash..
There was a problem hiding this comment.
Fixed. It doesn't makes sense to me to check for double-backslash, but I removed rstrip for backslash
| self, | ||
| path: str, | ||
| *, | ||
| params: Optional[Mapping[str, Union[str, int, float, bool, None, list, tuple]]] = None, |
| *, | ||
| json_body: Any = None, | ||
| data: Optional[Union[bytes, str]] = None, | ||
| params: Optional[Mapping[str, Union[str, int, float, bool, None, list, tuple]]] = None, |
There was a problem hiding this comment.
Union --> Optional[Union[...]]
redis/http/http_client.py
Outdated
| return resp.json() | ||
| return resp | ||
|
|
||
| def _prepare_body(self, *, json_body: Any = None, data: Optional[Union[bytes, str]] = None) -> Optional[Union[bytes, str]]: |
There was a problem hiding this comment.
We have a json type defined in redis/commands/json/_util.py. Can't it be reused for more detailed information about the acceptable input format?
There was a problem hiding this comment.
I think it's different. JsonType defined there allows to provide values like int, list, float because some commands allows you to append specific values to already existing Json object. In this case we expect a full Json object.
| def _build_url( | ||
| self, | ||
| path: str, | ||
| params: Optional[Mapping[str, Union[str, int, float, bool, None, list, tuple]]] = None, |
There was a problem hiding this comment.
Optional[Union[...]]
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
Please provide a description of the change here.