fixes issues 26 & 46. Added support for pagination #83
Conversation
Co-authored-by: Yevgeniy Brikman <[email protected]>
Used Regex to parse Link headers instead of splits and index arthemtic. getNextPath now returns `string` and `*FetchError`.
| return "", nil | ||
| } | ||
|
|
||
| nextLinkRegex, regexErr := regexp.Compile(`<(.+?)>;\s+rel="next"`) |
There was a problem hiding this comment.
In the regex here, it's rel="next" (in double quotes), but in the comment on line 217, it's rel=next (no double quotes). Which is correct?
There was a problem hiding this comment.
NIT: Also, this regex can be defined outside of this function once (e.g., at the top of github.go), with MustCompile, rather than each time this function is called.
| if regexErr != nil { | ||
| return "", wrapError(regexErr) | ||
| } | ||
| pathRegex, regexErr := regexp.Compile(`([^\/]+$)`) |
There was a problem hiding this comment.
NIT: same with compiling this one outside the function.
| } | ||
|
|
||
| for _, tc := range cases { | ||
| nextPath, err := getNextPath(tc.links) |
There was a problem hiding this comment.
NIT: wrap the body of the for-loop in:
t.Run("<UNIQUE TEST NAME>", func(t *testing.T) {
// The body
})That way, each test is run as a separate sub-test, and it's clearer what failed.
| if err != nil { | ||
| t.Fatalf("error getting next path: %s", err) | ||
| } |
There was a problem hiding this comment.
| if err != nil { | |
| t.Fatalf("error getting next path: %s", err) | |
| } | |
| require.NoError(t, err) |
| if nextPath != tc.expectedNextPath { | ||
| t.Fatalf("Expected next path %s, but got %s", tc.expectedNextPath, nextPath) | ||
| } |
There was a problem hiding this comment.
| if nextPath != tc.expectedNextPath { | |
| t.Fatalf("Expected next path %s, but got %s", tc.expectedNextPath, nextPath) | |
| } | |
| require.Equal(t, tc.expectedNextPath, nextPath) |
|
Alright, I may have to wrap this PR up myself... I'm going to create a new PR with your commits, and fix the final few issues on top of that. |
|
OK, I've opened #85 to wrap this up. Thanks! |
FetchTagsonly returns the first 30 tags sofetch --repo https://github.com/gruntwork-io/terratest --tag="<0.2.0" ./LocalDirfails with
... Tag does not existbecause terratest has 200+ tags and 0.2.0 isn't in the first 30 tags.I increased the
per_pageresult to 100 and added a loop to get the next link from the link header.I believe this fixes both issue #26 and #46.