-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
introduce AccessToken dataclass #128
Conversation
livechat/utils/structures.py
Outdated
@@ -30,3 +33,19 @@ def success(self) -> bool: | |||
def payload(self) -> dict: | |||
''' `payload` from the RTM response. ''' | |||
return self.rtm_response.get('payload') | |||
|
|||
|
|||
class Scheme(Enum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why scheme
and not type
? because of python's built-in type
?
edit:
also, it's a top-level definition without a direct link to the token, so i would consider TokenType
or sth. importing from livechat.utils.structures import Scheme
does not clearly indicate what scheme we are actually dealing with
livechat/utils/helpers.py
Outdated
for key, value in parameters.items() | ||
if key not in ['self', 'payload', 'headers', 'date_to', 'date_from'] | ||
for key, value in parameters.items() if key not in | ||
['self', 'payload', 'token', 'headers', 'date_to', 'date_from'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it requires adding:
if token:
payload['token'] = str(token)
several times. what is the reason here? do you think the current way of adding a token to the payload is too implicit or is it a matter of type conversion to str
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a trade-off, because JSON serializer doesn't know what to do with this AccessToken
object (and I haven't found easy way to make it json-serialazible easily in python), but I did it a bit differently in the post-review fixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, but add note to changelog
#126