code suggestion #25
Merged
code suggestion #25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
ccoVeille
commented
Dec 8, 2024
•
ccoVeille
commented
- chore: fix acronyms
- chore: use tb variable for testing.TB
- fix: potential Slowloris Attack
- refactor: restore the context, do not create one
- fix: use the expect Content-Type for OpenAPI
- chore: remove useless variables
- chore: rename method and variable to comply the acronym
- chore: use http constant
- test: fix inverted got-want
- test: make sure to close res.Body
- test: make sure to context is cancelled
- refactor: simplify panic recovery
- refactor: use type assertion and errors.Is to validate the server aborted
- refactor: do not use sprintf when sprint can be used
- refactor: invert if statement to make it clearer
this prevents to lose everything stored in the context
raeperd
reviewed
Dec 9, 2024
os.Exit called in TestMain will stop everything, defers are not launched
ccoVeille
force-pushed
the
code-suggestion
branch
from
10351fe to
5ead8d5
Compare
|
I applied the requested changes and suggestions |
raeperd
requested changes
Dec 9, 2024
| func handleGetOpenAPI(version string) http.HandlerFunc { | ||
| body := bytes.Replace(openAPI, []byte("${{ VERSION }}"), []byte(version), 1) | ||
| return func(w http.ResponseWriter, _ *http.Request) { | ||
| w.Header().Set("Content-Type", "text/yaml") |
There was a problem hiding this comment.
Suggested change
| w.Header().Set("Content-Type", "text/yaml") | |
| w.Header().Set("Content-Type", "application/yaml") |
There was a problem hiding this comment.
I'm too tired, indeed
There was a problem hiding this comment.
this type is not official, i'll fix it and merge this pr
| t.Parallel() | ||
| res, err := http.Get(endpoint + "/openapi.yaml") | ||
| testNil(t, err) | ||
| testEqual(t, http.StatusOK, res.StatusCode) | ||
| testEqual(t, "text/plain", res.Header.Get("Content-Type")) | ||
| testEqual(t, "text/yaml", res.Header.Get("Content-Type")) |
There was a problem hiding this comment.
Suggested change
| testEqual(t, "text/yaml", res.Header.Get("Content-Type")) | |
| testEqual(t, "application/yaml", res.Header.Get("Content-Type")) |
|
Thanks for detailed contribution! Handling panic this way is much cleaner and safe. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.