Share API code fixes, refactoring, docstrings, tests#9
Conversation
danil-topchiy
force-pushed
the
shares_fixes_docs_tests
branch
from
4ea0977 to
a5c2315
Compare
|
Hello @matejak |
danil-topchiy
changed the title
matejak
left a comment
matejak
left a comment
There was a problem hiding this comment.
Thank you for your work, it looks really good, especially those tests!
I have noticed some minor issues that were either present before and you just touched them, or that are also formal in their nature. Could you please take a look at them? Aside from those, the pull request is close to being merged.
| self.query_components.append("reshares=true") | ||
| Args: | ||
| path (str): path to file/folder | ||
| reshares (bool): (optional) return not only the shares from the current user but all shares from the given file |
There was a problem hiding this comment.
How is it with the bool type? We say that we expect bool, the default is None. Having to explicitly check for None makes the code a bit messy. Has None another meaning than False? This happens also to similar bool arguments later.
There was a problem hiding this comment.
Yes, None has different value than False. In most cases I convert True or False to string "true" or "false" and if it is None - this parameter not included in request.
There was a problem hiding this comment.
Should I add to description that this param is either bool or None?
There was a problem hiding this comment.
No, that description would have to be repeated everywhere, and usage of tribools like this is common. It should go to developer's documentation when we have one :-)
NextCloud.py
Outdated
| public_upload = "true" | ||
| if (path is None or not isinstance(share_type, int)) \ | ||
| or (share_type in [ShareType.GROUP, ShareType.USER, ShareType.FEDERATED_CLOUD_SHARE] | ||
| and share_with is None): |
There was a problem hiding this comment.
I would prefer this condition to be extracted to a utility function for sake of readability (e.g. share_parameters_dont_make_sense(path, share_type, share_with).
There was a problem hiding this comment.
Extracted it to staticmethod of Share class validate_share_parameters(path, share_type, share_with)
|
Good job, thank you for the PR! |
2 participants
Fixed non working methods of Share API, added docs, tests