#1 moved auth parsing to signin method #5
Conversation
|
🚀 means ship it. Make sure with the green merge button, click the down arrow and select Squash and Merge for the pull request. It's less important with this checkin, but if there is code review feedback, this will allow you to show a single merge. |
| site_id = parsed_response.find('.//t:site', namespaces=NAMESPACE).get('id', None) | ||
| user_id = parsed_response.find('.//t:user', namespaces=NAMESPACE).get('id', None) | ||
| auth_token = parsed_response.find('t:credentials', namespaces=NAMESPACE).get('token', None) | ||
| self.parent_srv._set_auth(site_id, user_id, auth_token) |
There was a problem hiding this comment.
Mostly style, but if this is getting called from outside the class/module, maybe it isn't really 'private' and doesn't need the '_' prefix.
There was a problem hiding this comment.
Our convention is any function call that is not intended for users to use should start with _. In this case, I think this fits that definition. @shinchris Do the users of the library ever have a handle to the endpoints? I don't remember
There was a problem hiding this comment.
Users can access the endpoints through the server class. Is that what you're asking?
Also I used the _ prefix because it's not intended for users to set the site_id, user_id, and auth_token.
There was a problem hiding this comment.
Makes sense -- I do like the convention, wasn't sure if it was supposed to be user-facing or not.
There was a problem hiding this comment.
@shinchris That's exactly what I was asking :) I thought that was the case, but couldn't remember off the top of my head. 🚀 🚀 🚀
|
🚀 Two small things |
No description provided.