Add FilesOptions to ListFiles, ListFilesReviewed; add missing fields.

[dmitshur]

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.

[dmitshur]

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.

Thoughts welcome.

I know it's not consistent with other endpoints. But I couldn't bring myself to return *[]string or *map[string]FileInfo, since I was making a breaking API change to the method anyway.

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.

[dmitshur]

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.

[codecov-io]

Codecov Report

Merging #52 into master will increase coverage by 1.1%. The diff coverage is 63.15%.

@@            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

Impacted Files	Coverage Δ
changes.go	16.14% <ø> (ø)	:arrow_up:
changes_revision.go	9.54% <63.15%> (+9.54%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 104d92e...ac10b04. Read the comment docs.

[andygrunwald]

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.

This sounds reasonable. And I think it is a good chance to go here.

All the other changes look quite good and also reasonable. Yep, breaking change. But this improves the behavior so far. Clients should use dep or another package manager to pin the version.

Thanks a lot!

[andygrunwald]

I created #53 for consistency.

Sorry for the long outstanding review. I was on vacation :D

[dmitshur]

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.