Conversation
| var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) | ||
|
|
||
| return response.Artifacts, nil | ||
| func findNextPage(resp *http.Response) string { | ||
| for _, m := range linkRE.FindAllStringSubmatch(resp.Header.Get("Link"), -1) { | ||
| if len(m) > 2 && m[2] == "next" { | ||
| return m[1] | ||
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Btw, I copied this code from elsewhere in the repository, and I would like to be able to reuse that instead of copying. I wasn't able to get import/referencing to work; I would try:
import (
...
cmdApi "github.com/cli/cli/v2/pkg/cmd/api"
)
...
cmdApi.findNextPage()but that would fail with findNextPage() is undefined in api package...
There was a problem hiding this comment.
It's perfectly fine to copy the function over. Lowercase functions in Go (e.g. findNextPage vs. FindNextPage) are private to a package and cannot be used from other packages, like you have found out when trying to invoke cmdApi.findNextPage().
|
would also appreciate advice or PR for testing the change. any pointers welcomed; new to golang |
mislav
left a comment
There was a problem hiding this comment.
This is looking good, but the code has inconsistent indentation. You can tell Go to format your code by saying go fmt ./pkg/cmd/run/shared. You can also configure your text editor to always format your Go code on file save (it's what I do).
As for testing, you can create a mock http.Client like this:
cli/pkg/cmd/run/shared/shared_test.go
Lines 40 to 47 in 2e6f202
You can then pass that client to ListArtifacts and use the registry to stub responses, like so
cli/pkg/search/searcher_test.go
Line 98 in 04ed77d
| var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) | ||
|
|
||
| return response.Artifacts, nil | ||
| func findNextPage(resp *http.Response) string { | ||
| for _, m := range linkRE.FindAllStringSubmatch(resp.Header.Get("Link"), -1) { | ||
| if len(m) > 2 && m[2] == "next" { | ||
| return m[1] | ||
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
It's perfectly fine to copy the function over. Lowercase functions in Go (e.g. findNextPage vs. FindNextPage) are private to a package and cannot be used from other packages, like you have found out when trying to invoke cmdApi.findNextPage().
|
@mislav with regards to |
mislav
left a comment
There was a problem hiding this comment.
Looks great! Thank you 🙇
Re: findNextPage duplication: you don't have to worry about it. If it gets too repetitive in our codebase, we will extract it separately. Thank you!
|
thanks for the merge, @mislav! glad to have my 1st OSS contribution here :) |
Fixes #5790
Testing
Used this workflow file to create 201 individual artifact uploads to the run, and was able to reproduce the bug not getting all of them when running
gh run download <run-id>.Compiled change, and then tested which was then able to get all 201 artifacts