-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add a trailing slash to URL when updating or deleting a file (tests fixed) #931
Conversation
@@ -2,7 +2,7 @@ https | |||
GET | |||
api.github.com | |||
None | |||
/repos/twitter/bootstrap/contents/js/bootstrap-affix.js | |||
/repos/twitter/bootstrap/contents//js/bootstrap-affix.js |
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 two //
?
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.
Because the requested path is appended to /contents/
, when a leading slash is provided, it results in two in the requested URL. Similarly, if one were to request /js//bootstrap-affix.js
, it would result in /repos/twitter/bootstrap/contents//js//bootstrap-affix.js
. There is no logic to remove duplicate /
in the URL, because it does not create an issue to have them.
If you would like this addressed, I can
- remove the leading slash from the test, which I dont think should be done, because the change would be intended to hide expected behavior
- update the code to remove leading slash or duplicate slashes, but that seems unnecessary as duplicate slashes do not alter behavior. Additionally, if a path with duplicate slashes were provided, it means the code alters the requested path before passing it along. The response would be unchanged, but it still feels wrong.
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.
Shouldn't we just remove the leading /
in:
PyGithub/github/tests/Issue140.py
Line 51 in 9d2129c
contents = self.repo.get_file_contents("/js/bootstrap-affix.js") |
@@ -2,7 +2,7 @@ https | |||
GET | |||
api.github.com | |||
None | |||
/repos/jacquev6/PyGithub/contents/ | |||
/repos/jacquev6/PyGithub/contents// |
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.
Same here, is it necessary?
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.
Not "necessary", just an artifact of how the issue is being addressed. Please see previous response.
@AetherDeity Thank you for taking this forward. Left some comments. Would also appreciate if you could add some example usage of |
@sfdye, hopefully I was able to address your comments. I have also added the requested example usages of ContentFile |
9f709a3
to
395d6eb
Compare
doc/examples/Repository.rst
Outdated
ContentFile(path="scripts") | ||
ContentFile(path="setup.py") | ||
|
||
Get all the contents of the repository |
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.
Add a "recursively"?
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.
fixed
doc/examples/Repository.rst
Outdated
@@ -49,3 +49,92 @@ Get all the labels of the repository | |||
Label(name="WIP") | |||
Label(name="bug") | |||
Label(name="documentation") | |||
|
|||
Get all the contents of a directory of the repository |
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.
You mean the "root directory"?
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.
fixed
|
||
>>> repo = g.get_repo("PyGithub/PyGithub") | ||
>>> contents = repo.get_contents("") | ||
>>> while len(contents) > 1: |
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.
PEP8 please :)
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.
oops, apparently I spend too much time writing yaml files anymore. Fixed
I think all the changes and tests look good now. Just a few comments on the example. |
7a288cb
to
4263fd3
Compare
4263fd3
to
b399dd4
Compare
Three birds with one stone, great work! @AetherDeity |
Thank you very much @AetherDeity for finishing up what I was struggling to finish. Thanks also @sfdye |
…ixed) (PyGithub#931) * Add a trailing slash to URL when updating or deleting a file. * Update test files to match new create/update/delete_file Repo methods * Add trailing slash to get_contents() method * update tests * add missing slash in get_dir_contents * add example usage * remove dup slashes in tests * clarify example comments
Closes #877
Closes #878
Closes #846
Address failing tests in #878