-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: add client.me property that calls v1/user (lazily) #51
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
src/posit/connect/client.py
Outdated
# Internal attributes to hold settings we fetch lazily | ||
self._server_settings = None | ||
self._current_user: Optional[User] = None |
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.
Since we are moving away from caching in Users
, should we remove it here for consistency?
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.
Good question. I think this is a reasonable one to cache because I don't expect it to change during the session, and like connect_version
, I expect that we'll have methods that internally will check something against the current user's (immutable) guid, so we wouldn't want to have to call the server each time. But you could rightly say that's YAGNI.
I don't feel strongly, what do you think? It's a minor change either way.
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.
Saving the GUID seems safe, but other information regarding the current_user
could change between calls to client.me
.
This morning, at least, I like the idea of not caching anything by convention. We can add it back in if the server request load becomes burdensome.
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.
Yep, I agree. I'll remove, then merge.
Replacement for #33, was easier to pull the changes to a fresh branch than try to rebase
Fixes #22.