refactor: type hints private interface #389
Conversation
| blocking_timeout: Optional[int] = None, | ||
| url: Optional[str] = None, | ||
| url_template: Optional[str] = None, | ||
| logger: Optional[optimizely_logger.Logger] = None, |
There was a problem hiding this comment.
I am not sure, is the type in the past we were expecting?
There was a problem hiding this comment.
This is just a type alias for the union of the python standard library logging.Logger and our BaseLogger.
| variables: Optional[dict[str, Any]] = None, | ||
| rule_key: Optional[str] = None, | ||
| flag_key: Optional[str] = None, | ||
| user_context: Optional[OptimizelyUserContext] = None, |
There was a problem hiding this comment.
this shouldn't be NULL.
There was a problem hiding this comment.
The only problem with removing the user_context default None, is that we can't have a non default argument after default arguments. So to remove the user_context default, we would need to make user_context the first parameter or remove all preceding defaults, which would both be breaking changes.
| @@ -16,21 +16,21 @@ | |||
|
|
|||
| https://pypi.python.org/pypi/mmh3/2.3.1 | |||
| ''' | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
I am leaving this file unreviewed.
| @@ -624,7 +629,7 @@ def get_variation( | |||
|
|
|||
| return variation_key | |||
|
|
|||
| def is_feature_enabled(self, feature_key: str, user_id: str, attributes: Optional[dict[str, Any]] = None) -> bool: | |||
| def is_feature_enabled(self, feature_key: str, user_id: str, attributes: Optional[UserAttributes] = None) -> bool: | |||
There was a problem hiding this comment.
Can you please confirm, this won't break any code change.
There was a problem hiding this comment.
This is essentially a no-op wrapper, that gives it a name for the type checker to track. But at runtime the behavior is unchanged.
Summary