Improve DownloadContents and DownloadContentsWithMeta methods #3573
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Ah, I see now what is happening. Your PR is changing how downloads work... which is probably not a good sign in that this change as written will cause compatibility problems with existing code as demonstrated in the go-github/tools/metadata/main_test.go Lines 356 to 378 in ef319d5 It looks like you are going to have to make it so that your changes do not adversely affect existing code, if possible. If it is not possible, then we probably are going to have to keep the legacy behavior consistent for existing users and instead add a new endpoint that behaves the way you propose, with detailed explanations in comments as to when to use the legacy mechanism and when to use your new mechanism. Does that make sense? |
I believe I’ve finally managed to fix the problem. Take a look when you have a moment. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3573 +/- ##
==========================================
+ Coverage 91.23% 91.29% +0.05%
==========================================
Files 183 183
Lines 16053 16087 +34
==========================================
+ Hits 14646 14686 +40
+ Misses 1231 1227 -4
+ Partials 176 174 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thank you, @kuzminT!
Two tiny nits to address, please, otherwise looks good.
If you could increase the code coverage, that would be great, but not critical.
After the nits, we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.
There was a problem hiding this comment.
Thank you, @kuzminT!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
@stevehipwell - might you have time for a code review? Thank you!
|
Thank you, @stevehipwell! |
Fixes: #3566.
By parsing the content field from the response, we can retrieve the most recent state of the file without making an additional request. Only when the content field is empty—such as for object format and files exceeding the 1 MB limit (see GitHub API docs) — do we fall back to the previous approach, which involves directory parsing and fetching the content via download_url or as raw data.
Additional tests were added to cover various cases.