Add FilesOptions to ListFiles, ListFilesReviewed; add missing fields. #52
Conversation
Thoughts welcome. I know it's not consistent with other endpoints. But I couldn't bring myself to return I think we should change all other methods that similarly return pointers to maps to be return just map values. If that's the agreed direction, doing this first step here makes sense. Otherwise, I should revert it. |
Modify ListFilesReviewed to return a slice of strings, rather than slice of FileInfos. This matches its actual behavior. Modify both ListFiles and ListFilesReviewed to return map/slice directly, rather than a pointer to one. There doesn't appear to be any value in returning a pointer, it just makes the API harder to use. Slice/map are already reference types. Modify all endpoints to escape fileID parameter so it can be safely placed inside a URL path segment. Make note of Base parameter being undocumented for ListFiles endpoints. However, it has been tested and it works (it's a very important parameter to support). Add tests for ListFiles, ListFilesReviewed.
It's needed for url.PathEscape. Add Go 1.10.x to CI. Rearrange versions in CI so that latest version is tested first, and older versions are tested afterwards.
dmitshur
force-pushed
the
add-FilesOptions-etc
branch
from
92e4a4d to
8ab693e
Compare
|
I've also bumped the minimum version tested from 1.6 to 1.8. Given that 1.10 is out now, supporting it and two previous versions seems reasonable. Let me know if you feel strongly about supporting 1.6 and 1.7, it can be arranged. |
This change fixes the following vet issue: $ go vet # github.com/andygrunwald/go-gerrit_test ./changes_test.go:39: ExampleChangesService_QueryChangesWithSymbols refers to unknown field or method: ChangesService.QueryChangesWithSymbols The example naming conventions are documented at https://godoc.org/testing#hdr-Examples.
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
=========================================
+ Coverage 21.23% 22.34% +1.1%
=========================================
Files 21 21
Lines 1775 1781 +6
=========================================
+ Hits 377 398 +21
+ Misses 1353 1332 -21
- Partials 45 51 +6
Continue to review full report at Codecov.
|
This sounds reasonable. And I think it is a good chance to go here. All the other changes look quite good and also reasonable. Thanks a lot! |
|
I created #53 for consistency. Sorry for the long outstanding review. I was on vacation :D |
|
On the contrary, this was quite fast, especially compared to the number of months it took me to upstream this change from my local repository. :) Thank you for the review. |
Modify ListFilesReviewed to return a slice of strings, rather than slice of FileInfos. This matches its actual behavior (according to documentation; I haven't tested against a real API because it requires auth).
Modify both ListFiles and ListFilesReviewed to return map/slice directly, rather than a pointer to one. There doesn't appear to be any value in returning a pointer, it just makes the API harder to use. Slice/map are already reference types.
Modify all endpoints to escape fileID parameter so it can be safely placed inside a URL path segment.
Make note of Base parameter being undocumented for ListFiles endpoints. However, it has been tested and it works (it's a very important parameter to support).
Add missing fields to ChangeMessageInfo, FileInfo.
Add tests for ListFiles, ListFilesReviewed.