Skip to content
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: set user attributes on update #119

Merged
merged 5 commits into from
Mar 21, 2024
Merged

feat: set user attributes on update #119

merged 5 commits into from
Mar 21, 2024

Conversation

tdstein
Copy link
Collaborator

@tdstein tdstein commented Mar 20, 2024

Also fixes issues with client.me attribute and uses the correct HTTP method

Resolves #117
Resolves #99

Copy link

github-actions bot commented Mar 20, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
527 439 83% 80% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
src/posit/connect/client.py 100% 🟢
src/posit/connect/users.py 91% 🟢
TOTAL 96% 🟢

updated for commit: 3451dec by action🐍

Copy link
Collaborator

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of suggestions but otherwise LGTM

# TODO(#99): that patch request returns a payload on success,
# so we should instead update the local object with that payload
# (includes updated_time)
if len(body) == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we test this case?

src/posit/connect/client.py Show resolved Hide resolved
src/posit/connect/client.py Show resolved Hide resolved
src/posit/connect/client.py Outdated Show resolved Hide resolved
@tdstein tdstein merged commit 359965c into main Mar 21, 2024
7 checks passed
@tdstein tdstein deleted the tdstein/117 branch March 21, 2024 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the API response to update the local object Finish adding .update() to User
2 participants