fix: types related to message body should be bytes #132
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
fffonion
reviewed
Oct 20, 2022
mayocream
reviewed
Oct 21, 2022
| @@ -220,7 +220,7 @@ func (r Response) SetHeaders(headers map[string][]string) error { | |||
| // Unless manually specified, this method will automatically set the | |||
| // Content-Length header in the produced response for convenience. | |||
|
|
|||
| func (r Response) Exit(status int, body string, headers map[string][]string) { | |||
| func (r Response) Exit(status int, body []byte, headers map[string][]string) { | |||
There was a problem hiding this comment.
Is this a breaking change? I know it’s expected to be the bytes type, but someone may rely on the string type already.
Should we confirm that which version should this be?
Or can we make it more smarter: accept a interface and transform string to bytes instead internally.
There was a problem hiding this comment.
Good idea. In this way, we gain the ability to use the new interface but also keep the old code working. But we also have some functions that are returning string, which we want to change to bytes. The trick does not work with returning values, and if we only do this to parameters, it will be inconsistent.
We do need to inform users about this breaking change.
fffonion
approved these changes
Dec 7, 2022
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.
Rerun test after merging Kong/kong#9526
Fix Kong/kong#9294