-
Notifications
You must be signed in to change notification settings - Fork 39
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
V7 2428 harmonize api for clientpy #287
Conversation
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.
See comments
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.
Minor comment, great work!
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.
Requested a few changes already, but pretty much:
- in the new client functions, I would try and be consistent by always returning the
response
- there are some naming conventions I personally don't like (
the_{}
) - we are not really introducing any tests for
client.py
- some things feel a bit weird, like using
cast
or importinglogging
in all tests. Are we sure there is no other way?
darwin/client.py
Outdated
team_slug: str | ||
Team slug of the dataset. | ||
""" | ||
self._put(f"datasets/{dataset_id}/archive", payload={}, team=team_slug) |
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.
We're probably not using the response when we call archive_remote_dataset
, but I would actually return the response for API consistency. What do you think?
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.
I would go even further and actually use a DatasetIdentifier
iin every place we are using a slug or an id
. However, I think I made enough breaking changes as is for 1 PR :D
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.
Ticket created for this topic:
https://linear.app/v7labs/issue/V7-2547/harmonization-part-2
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.
Approved!
Background
Implementation of changes from RFC:
https://www.notion.so/v7labs/API-Harmonization-for-client-py-0248a627bad144109bf64f078134cecf
This PR focuses on:
reponse.ok
instead of checking forresponse.status_code != 200
Missing: