Refactored api.register #260
Conversation
|
Thanks for the patch. Signoff please (Haven't yet reviewed). |
|
Signed-off-by: Abhishek Kumar abhishekkumar2718@gmail.com |
non-Jedi
left a comment
non-Jedi
left a comment
There was a problem hiding this comment.
Other than small change, this looks good! Thanks for contributing. :)
matrix_client/api.py
Outdated
| self.validate_cert = valid | ||
|
|
||
| def register(self, content=None, kind='user'): | ||
| def register(self, auth_body, kind, **kwargs): |
There was a problem hiding this comment.
Let's explicitly list all the possible kwargs here. bind_email, username, password device_id, and initial_device_display_name.
There was a problem hiding this comment.
Not sure if it was clear before. I want the following listed in the function signature; not just the docstring. :)
There was a problem hiding this comment.
Wouldn't that defeat the point of using kwargs ?
I think you mean a function signature like register(self, auth_body, bind_email=None, ...)
There was a problem hiding this comment.
Yep. I think this method would be better without kwargs with each possible top-level key listed explicitly. Just like you showed, yes.
non-Jedi
left a comment
non-Jedi
left a comment
There was a problem hiding this comment.
Getting a lot closer. :)
matrix_client/api.py
Outdated
| auth_body (dict): Authentication Params. | ||
| kind (str): Specify kind of account to register. | ||
| bind_email (bool): Whether to use email in registration and authenication | ||
| username (str): The localpart of an Matrix ID. |
There was a problem hiding this comment.
should be "a Matrix ID" not "an"
matrix_client/api.py
Outdated
| if content is None: | ||
| auth_body (dict): Authentication Params. | ||
| kind (str): Specify kind of account to register. | ||
| bind_email (bool): Whether to use email in registration and authenication |
There was a problem hiding this comment.
spelling: "authentication"
matrix_client/api.py
Outdated
| """ | ||
| if content is None: | ||
| auth_body (dict): Authentication Params. | ||
| kind (str): Specify kind of account to register. |
There was a problem hiding this comment.
Need to note that this is one of "guest" or "user".
matrix_client/api.py
Outdated
| "inhibit_login": inhibit_login, | ||
| "bind_email": bind_email, | ||
| } | ||
| if username: |
There was a problem hiding this comment.
If no auth_body is given, content won't exist yet. Need to instantiate content before any of these if statements.
matrix_client/api.py
Outdated
| initial_device_display_name (str): Display name to be assigned. | ||
| """ | ||
| if auth_body: | ||
| content = { |
There was a problem hiding this comment.
This should just be if auth_body: content["auth"] = auth_body
matrix_client/api.py
Outdated
| def register(self, content=None, kind='user'): | ||
| def register(self, auth_body=None, kind="user", bind_email=False, | ||
| username=None, password=None, device_id=None, | ||
| initial_device_display_name=None, inhibit_login=False): |
There was a problem hiding this comment.
Please set bind_email and inhibit_login to None default values. If user doesn't explicitly set them, body of JSON sent shouldn't include those keys.
| content["device_id"] = device_id | ||
| if initial_device_display_name: | ||
| content["initial_device_display_name"] = \ | ||
| initial_device_display_name |
There was a problem hiding this comment.
Need to add similar statements to above for bind_email and inhibit_login.
|
LGTM. If you can squash down to one commit, I'll go ahead and merge. Thanks! |
Zil0
left a comment
Zil0
left a comment
There was a problem hiding this comment.
Two small things and then we'll be good. Thanks!
| username (str): The localpart of a Matrix ID. | ||
| password (str): The desired password of the account. | ||
| device_id (str): ID of the client device. | ||
| initial_device_display_name (str): Display name to be assigned. |
There was a problem hiding this comment.
This list lacks inhibit_login.
matrix_client/api.py
Outdated
| Args: | ||
| login_type (str): The value for the 'type' key. | ||
| **kwargs: Additional key/values to add to the JSON submitted. | ||
| **kwargs: Additional key/values to add to JSON submitted. |
Refactored api.register to use auth_body and **kwargs. Fixed silly PEP8 mistakes. Expanded keyword arguments in the doc string Fixed PEP8 Trailing Whitespace error Explicitly listed key word arguments Explicitly listed keyword arguments Final finishes Refactored api.register to use auth_body and **kwargs Minor documentation fixes. Refactored api.register to use auth_body and **kwargs.
|
Merged, thanks for the time you spent on this :) |
|
It was my pleasure! Onto the next open issue. :) |
3 participants
api.register now uses auth_body, kind and **kwargs.
Closes #213