-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update unity_sds_client with Data Service Delete #52
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the delete you have is fine (but very manual), but the name of it is not great. When i saw it i thought it would delete all the data in the collection.
Any methods created need to include the docstring that describes them. See https://github.com/unity-sds/unity-monorepo/blob/feature/unity-sds-client-update-ds-delete/libs/unity-py/unity_sds_client/resources/collection.py#L90-L99 as an example. without this peope won't really know how to use this method other than digging into the source code. Documentation ends up at https://unity-sds.github.io/unity-monorepo/unity-sds-client/ Both |
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.
Why not query for the specific collection we're going to delete from? getting a listing and then choosing the first (with no idea what it is?) is hardly a test. Does an error get thrown if we request a delete and the file doesn't exist?
For a test like this, i'd expect us to:
find/create a collection
add a file to it
check to see if that file exists
delete the file
ensure its deleted
delete the collection on cleanup. this is certainly an integration test, and i'm not sure we can do all of that right now (maybe talk to Nga) but how you're running this test is not going to be sufficient.