-
Notifications
You must be signed in to change notification settings - Fork 20
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
Introduce PatchType #158
Introduce PatchType #158
Conversation
I like the idea, go for it :) 👍 |
Use this to distinguish different patch types, but only on patch_record. Other patch_methods can come later if the technique seems sane.
This makes it easier to write these kinds of PATCH methods.
This is analogous to patch_record, except that even if no ID is present, we can still sometimes succeed because we have a _bucket_name attribute.
Also, add a test, since one isn't present.
This argument in update_bucket, update_group, update_collection, and update_record only existed to support the patch methods, as far as I can tell, since it's completely untested. Since it no longer makes sense to pass anything but ``PUT`` here, take it out.
Also, wrong patches should be TypeError, not ValueError.
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 don't have sphinx/autodocs, don't forget to update the readme too ;)
README.rst
Outdated
|
||
# It is also possible to manipulate bucket permissions (see later) | ||
client.patch_bucket(id='payments', permissions={}) | ||
client.patch_bucket(id='payments', data=BasicPatch({'status': 'updated'}), permissions={}) |
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 wasn't sure how to update this, but the given example wouldn't work with the new code. There's no tests for this kind of behavior either. If we intend for it to work even when you don't pass data at all, we should add tests for it.
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 agree, if there is no test then it does not exist.
However, I believe this is a relevant use-case.
You can remove this from the README and open an issue to support it.
I just realized that this only supports patching |
While we're here, make the pytest.raises assertions a little stricter.
Permissions have to be part of a patch_type because they are covered by the same Content-Type. This also means we can handle updating just permissions without updating the data (fixes #161).
This verifies that it actually works with the server!
This is a proof-of-concept to demonstrate my opinion on the preferred way to address #125. I only updated patch_record so far, but adding others should be straightforward.