refactor(fetcher): minor refactoring #3752
Conversation
jvoisin
commented
Aug 29, 2025
jvoisin
commented
- Replace a strings.HasPrefix + strings.Cut with a call to strings.CutPrefix.
- Instead of enumerating all possible redirection code, check if the code is 300-something.
- Extract an error constructor outside of its current scope instead of duplicating it 5 times
- Use constant strings for LocalizedError's return values where possible.
There was a problem hiding this comment.
Pull Request Overview
This PR applies several minor refactoring improvements to the response handler fetcher code to make it more readable and maintainable.
- Modernizes string processing by using
strings.CutPrefixandstrings.SplitSeq - Simplifies HTTP redirect detection with status code range checking
- Eliminates code duplication by extracting error construction logic
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Replace a strings.HasPrefix + strings.Cut with a call to strings.CutPrefix. - Instead of enumerating all possible redirection code, check if the code is 300-something. - Extract an error constructor outside of its current scope instead of duplicating it 5 times - Use constant strings for LocalizedError's return values where possible.
jvoisin
force-pushed
the
resphandlerpass
branch
from
4ff449e to
ff5ff7a
Compare
| r.httpResponse.StatusCode == http.StatusSeeOther || | ||
| r.httpResponse.StatusCode == http.StatusTemporaryRedirect || | ||
| r.httpResponse.StatusCode == http.StatusPermanentRedirect) | ||
| return r.httpResponse != nil && r.httpResponse.StatusCode >= 300 && r.httpResponse.StatusCode < 400 |
There was a problem hiding this comment.
This is not correct because some status codes in the 3xx range are not redirects. For example: 304 / StatusNotModified is not a redirect.
There was a problem hiding this comment.
It's technically a redirect, as it's a redirection to the local cache, which shouldn't happen in our use-case anyway, since the only place we're making use of IsRedirect is for // Do not add redirections to the possible list of subscriptions to avoid confusion.
2 participants
