[api] Clean up `--rate-limit` implementation

commands/api.go

@@ -250,8 +250,7 @@ func apiCommand(_ *Command, args *Args) {
 
 	args.NoForward()
 
-	requestLoop := true
-	for requestLoop {
+	for {
 		response, err := gh.GenericAPIRequest(method, path, body, headers, cacheTTL)
 		utils.Check(err)
 		success := response.StatusCode < 300
@@ -285,7 +284,6 @@ func apiCommand(_ *Command, args *Args) {
 			os.Exit(22)
 		}
 
-		requestLoop = false
 		if paginate {
 			if isGraphQL && hasNextPage && endCursor != "" {
 				if v, ok := params["variables"]; ok {
@@ -295,47 +293,32 @@ func apiCommand(_ *Command, args *Args) {
 					variables := map[string]interface{}{"endCursor": endCursor}
 					params["variables"] = variables
 				}
-				requestLoop = true
+				goto next
 			} else if nextLink := response.Link("next"); nextLink != "" {
 				path = nextLink
-				requestLoop = true
+				goto next
 			}
 		}
-		if requestLoop && !parseJSON {
+
+		break
+	next:
+		if !parseJSON {
 			fmt.Fprintf(out, "\n")
 		}
 
-		if requestLoop && rateLimit {
-			var rateLimitLeft = -1
-			var rateLimitResetMs = -1
-			var atoiErr error
-			if xRateLimitLeft := response.Header.Get(rateLimitRemainingHeader); len(xRateLimitLeft) != 0 {
-				if rateLimitLeft, atoiErr = strconv.Atoi(xRateLimitLeft); atoiErr != nil {
-					ui.Errorf("Unable to parse header \"%s\": \"%s\" as an int", rateLimitRemainingHeader, xRateLimitLeft)
-					rateLimitLeft = -1
-				}
-			}
-			if xRateLimitReset := response.Header.Get(rateLimitResetHeader); len(xRateLimitReset) != 0 {
-				if rateLimitResetMs, atoiErr = strconv.Atoi(xRateLimitReset); atoiErr != nil {
-					ui.Errorf("Unable to parse header \"%s\": \"%s\" as an int", rateLimitResetHeader, xRateLimitReset)
-					rateLimitResetMs = -1
-				}
-			}
-			if rateLimitResetMs != -1 && rateLimitLeft == 0 {
-				rollover := time.Unix(int64(rateLimitResetMs)+1, 0)
-				ui.Errorf("Pausing until %v for Rate Limit Reset ...\n", rollover)
-				time.Sleep(time.Until(rollover))
-			}
+		if rateLimit && response.RateLimitRemaining() == 0 {
+			resetAt := response.RateLimitReset()
+			rollover := time.Unix(int64(resetAt)+1, 0)
+			ui.Errorf("API rate limit reached; pausing until %v ...\n", rollover)
+			time.Sleep(time.Until(rollover))
 		}
 	}
 }
 
 const (
-	trueVal                  = "true"
-	falseVal                 = "false"
-	nilVal                   = "null"
-	rateLimitRemainingHeader = "X-Ratelimit-Remaining"
-	rateLimitResetHeader     = "X-Ratelimit-Reset"
+	trueVal  = "true"
+	falseVal = "false"
+	nilVal   = "null"
 )
 
 func magicValue(value string) interface{} {

features/api.feature

@@ -422,75 +422,18 @@ Feature: hub api
     When I run `hub api -t count --cache 5`
     Then it should pass with ".count	2"
 
-  Scenario: Honor rate limit by sleeping
+  Scenario: Honor rate limit with pagination
     Given the GitHub API server:
       """
       get('/hello') {
         page = (params[:page] || 1).to_i
-        response.headers['X-Ratelimit-Remaining'] = '0'
-        # it doesn't matter, cucumber blanks the .sleep anyway
-        response.headers['X-Ratelimit-Reset'] = '1'
-        response.headers['Link'] = %(</hello?page=2>; rel="next") if page < 2
+        if page < 2
+          response.headers['X-Ratelimit-Remaining'] = '0'
+          response.headers['X-Ratelimit-Reset'] = Time.now.utc.to_i.to_s
+          response.headers['Link'] = %(</hello?page=#{page+1}>; rel="next")
+        end
         json [{}]
       }
       """
-    When I run `hub api --rate-limit --paginate hello`
-    Then the stderr should contain "Pausing until "
-
-    Given the GitHub API server:
-      """
-      get('/hello') {
-        page = (params[:page] || 1).to_i
-        response.headers['X-Ratelimit-Remaining'] = '0'
-        # it doesn't matter, cucumber blanks the .sleep anyway
-        response.headers['X-Ratelimit-Reset'] = '9999999999'
-        response.headers['Link'] = %(</hello2>; rel="next")
-        json [{}]
-      }
-      get('/hello2') {
-        status 403
-      }
-      """
-    When I run `hub api --paginate hello`
-    Then the exit status should be 22
-    And the stderr should contain exactly ""
-
-    Given the GitHub API server:
-      """
-      get('/hello') {
-        page = (params[:page] || 1).to_i
-        response.headers['X-Ratelimit-Remaining'] = 'hello world'
-        response.headers['X-Ratelimit-Reset'] = 'yankee doodle'
-        response.headers['Link'] = %(</hello2>; rel="next")
-        json [{:page => 1}]
-      }
-      get('/hello2') {
-        json [{:page => 2}]
-      }
-      """
-    When I run `hub api --rate-limit --paginate hello`
-    Then the stdout should contain exactly:
-      """
-      [{"page":1}]
-      [{"page":2}]
-      """
-    And the stderr should contain "Unable to parse"
-
-    Given the GitHub API server:
-      """
-      get('/hello') {
-        response.headers['Link'] = %(</hello2>; rel="next")
-        json [{:page => 1}]
-      }
-      get('/hello2') {
-        json [{:page => 2}]
-      }
-      """
-    # rate-limit should behave rationally even when missing the headers
-    When I run `hub api --rate-limit --paginate hello`
-    Then the stdout should contain exactly:
-      """
-      [{"page":1}]
-      [{"page":2}]
-      """
-    And the stderr should contain exactly ""
+    When I successfully run `hub api --rate-limit --paginate hello`
+    Then the stderr should contain "API rate limit reached; pausing until "

github/http.go

@@ -32,6 +32,11 @@ const checksType = "application/vnd.github.antiope-preview+json;charset=utf-8"
 const draftsType = "application/vnd.github.shadow-cat-preview+json;charset=utf-8"
 const cacheVersion = 2
 
+const (
+	rateLimitRemainingHeader = "X-Ratelimit-Remaining"
+	rateLimitResetHeader     = "X-Ratelimit-Reset"
+)
+
 var inspectHeaders = []string{
 	"Authorization",
 	"X-GitHub-OTP",
@@ -517,3 +522,21 @@ func (res *simpleResponse) Link(name string) string {
 	}
 	return ""
 }
+
+func (res *simpleResponse) RateLimitRemaining() int {
+	if v := res.Header.Get(rateLimitRemainingHeader); len(v) > 0 {
+		if num, err := strconv.Atoi(v); err == nil {
+			return num
+		}
+	}
+	return -1
+}
+
+func (res *simpleResponse) RateLimitReset() int {
+	if v := res.Header.Get(rateLimitResetHeader); len(v) > 0 {
+		if ts, err := strconv.Atoi(v); err == nil {
+			return ts
+		}
+	}
+	return -1
+}