Support to download repository content (zip/tar)#152
Conversation
|
Hi @miwurster. We apologise for taking so long to get eyes on this PR, we haven't been able to maintain this library to the standard we would have liked to but this will be changing in the new year. If you could please get this rebased and we'll be prioritising all open PRs ready for review. Thanks! Ellie |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #152 +/- ##
============================================
+ Coverage 76.64% 76.84% +0.20%
- Complexity 289 296 +7
============================================
Files 42 42
Lines 989 1002 +13
Branches 43 44 +1
============================================
+ Hits 758 770 +12
- Misses 206 207 +1
Partials 25 25 ☔ View full report in Codecov by Sentry. |
- separate methods to download the tar or zip archive - methods return an Optional InputStream that can be consumed
|
Rebased and ready for review. |
felix-seifert
left a comment
felix-seifert
left a comment
There was a problem hiding this comment.
Nice feature addition. 😀 You should just avoid the usage of null. In this repo, we might be using null every now and then. However, we should really change this. 🙈
| .request(new Request.Builder().url("https://example.com/whatever").build()) | ||
| .protocol(Protocol.HTTP_1_1) | ||
| .message("") | ||
| .code(204) // No Content |
There was a problem hiding this comment.
Nice that you described the status code with a short comment. 👍 (No need to change anything)
| } | ||
|
|
||
| private CompletableFuture<Optional<InputStream>> downloadRepository(final String path, final String ref) { | ||
| final var repoRef = Strings.nullToEmpty(ref); |
There was a problem hiding this comment.
Having to handle null is not nice and might introduce bugs in future iterations. The obvious goto would be Optional. However, it might not always be the nicest in method calls. Furthermore, using it makes the method overloading redundant.
My suggestion here is to use Optional only for your private method: downloadRepository(final String path, final Optional<String> maybeRef). Your repoRef could then become something like maybeRef.orElse("").
In methods like downloadZipball(), you would then not call the same method with null but downloadRepository(REPOSITORY_DOWNLOAD_ZIPBALL, Optional.empty()).
There was a problem hiding this comment.
Yeah sure, I'll change that after lunch :-)
|
Implemented review comments |
felix-seifert
left a comment
felix-seifert
left a comment
There was a problem hiding this comment.
I was wondering whether we should make it more obvious that we test for specific refs. Currently, we have this tested in shouldReturnEmptyOptionalWhenResponseBodyNotPresent(), i.e. the request has to work for the ref master. However, this test does not explain this specifically.
Might be interesting to test this but I think it is not worth the effort.
3 participants
Hi, this PR adds support to download the repository content as zip or tar archive.
I extended the Repository client with two separate methods to download the tar or zip archive. The methods return an Optional InputStream that can be consumed accordingly, e.g., by a
BufferedInputStreamthat writes it read bytes to aFileOutputStreamor by following this guide.