-
Notifications
You must be signed in to change notification settings - Fork 220
Cloneable #154
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
Conversation
- Decorate Folder's methods with api_call - Implement clone in BaseObject
- Decorate Folder's methods with api_call - Implement clone in BaseObject
|
Verified that @kelseymorris95 has signed the CLA. Thanks for the pull request! |
| from ..object.file import File | ||
| from ..object.group import Group | ||
| from ..object.group_membership import GroupMembership | ||
| from ..object.cloneable import Cloneable |
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.
alphabetize
|
👍 |
boxsdk/object/cloneable.py
Outdated
| different session member if desired. | ||
| """ | ||
|
|
||
| def __init__(self, *args, **kwargs): |
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.
This override isn't necessary, since all it does is call super.
Moved as_user() and with_shared_link() implementations to Cloneable, added session propertry.
|
|
||
| def with_shared_link(self, shared_link, shared_link_password): | ||
| """ | ||
| Returns a new endpoint object with default headers set up to make requests using the shared link for auth. |
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.
Sphinx needs blank lines between the title, and the rest of the docstring.
This is fine for now, but we should probably do a test run of sphinx, and fix all the errors, before doing a release.
|
Looks good to me! There are a few minor things I pointed out here, but we can deal with them in another PR. 👍 |
| return self.__class__(self._oauth, self._network_layer, headers, self._default_network_request_kwargs.copy()) | ||
|
|
||
| def with_default_network_request_kwargs(self, extra_network_parameters): | ||
| return self.__class__(self._oauth, self._network_layer, self._default_headers.copy(), extra_network_parameters) |
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.
Let's chat about this later:
Since this is the only way for objects to specify request_kwargs, I think it makes more sense for this to be called with_network_request_kwargs (drop the default).
I also feel like the default behavior should be to update any existing arguments, not replace them. Otherwise, it's sort of a last-call-wins, and the "default" arguments only have a meaning if there are no more calls to this method.
Includes proof of concept from https://github.com/Jeff-Meadows/box-python-sdk/tree/api_wrapper. New api_call decorator can be used to allow methods that make an API call to accept an extra parameter, to be used for sending low level request parameters to Requests or whatever network layer is being used.