Skip to content

cmd error parsing: show a "try again after" message when a response sets a Retry-After header #1198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 19, 2025

Conversation

IsaacG
Copy link
Member

@IsaacG IsaacG commented Jun 19, 2025

No description provided.

@iHiD iHiD requested review from ErikSchierboom and Copilot June 19, 2025 05:13
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates error response handling to display a "try again after" message when an API returns a Retry-After header, and refines test helpers for different error status codes.

  • Renames the existing error response helper to errorResponse418 for 418 errors.
  • Adds a new error response helper errorResponse429 for 429 responses with a Retry-After header.
  • Updates decodedAPIError to check for and appropriately format the Retry-After header in error messages.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
cmd/cmd_test.go Updated helper functions and tests to simulate 418 and 429 responses.
cmd/cmd.go Added logic in decodedAPIError to format Retry-After values in errors.
Comments suppressed due to low confidence (1)

cmd/cmd.go:85

  • Consider adding a brief comment here clarifying that if the Retry-After header cannot be parsed as an integer, its value is assumed to be a pre-formatted HTTP date.
	if retryAfter := resp.Header.Get("Retry-After"); retryAfter != "" {

@iHiD
Copy link
Member

iHiD commented Jun 19, 2025

Looks good to me. Thank you. I'd like another pair of eyes before merging though

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ErikSchierboom ErikSchierboom merged commit 19a7972 into exercism:main Jun 19, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants