Skip to content

Conversation

gutenye
Copy link

@gutenye gutenye commented Sep 12, 2025

cause res.statusText can be undefined

SCR-20250912-lylk

@gutenye gutenye requested review from sparten11740, a team and Copilot September 12, 2025 05:24
@gutenye gutenye self-assigned this Sep 12, 2025
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 fixes an issue where error messages could be incomplete when res.statusText is undefined by including the HTTP status code in error messages.

  • Updates error message construction to include both status code and status text
  • Handles cases where res.statusText may be undefined by providing a fallback

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

}

const err = new Error(res.statusText)
const err = new Error(`${res.status} ${res.statusText || ''}`)
Copy link
Preview

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

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

The error message format could result in trailing whitespace when res.statusText is undefined. Consider using a more explicit format like ${res.status}${res.statusText ? ': ' + res.statusText : ''} to avoid unnecessary spaces.

Suggested change
const err = new Error(`${res.status} ${res.statusText || ''}`)
const err = new Error(`${res.status}${res.statusText ? ': ' + res.statusText : ''}`)

Copilot uses AI. Check for mistakes.

@gutenye gutenye requested a review from ChALkeR September 12, 2025 05:26
@akinncar
Copy link

akinncar commented Oct 2, 2025

cause res.statusText can be undefined

if statusText is currently undefined, in this new implementation, statusText will be end up as the same as status, right?
so why do we need this?

@gutenye
Copy link
Author

gutenye commented Oct 6, 2025

if statusText is currently undefined, in this new implementation, statusText will be end up as the same as status, right? so why do we need this?

no, before the error message is empty, after the error message is status, at lease it's not empty.

Copy link

@dagohan dagohan left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Contributor

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

this is a frozen fetchival impl
replacement api is in experimental/ dir to ensure compat

this file will be removed in a major release, see #25

any changes that are exclusive to the file being removed don't make sense

@gutenye
Copy link
Author

gutenye commented Oct 7, 2025

@gutenye gutenye closed this Oct 7, 2025
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.

5 participants