Skip to content
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

feat: PostResponse supports charset other than UTF-8 #239

Merged
merged 3 commits into from May 22, 2023

Conversation

joyyir
Copy link
Contributor

@joyyir joyyir commented May 11, 2023

Please answer these questions before submitting a pull request


Bugfix

  • Description

  • How to fix?


New feature or improvement

  • Describe the details and related test reports.
    PostResponse by default converts the body to bytes in UTF-8.
    It would be nice to support other charsets.

  • Source branch
    main

  • Related commits and pull requests

  • Target branch
    main

@joyyir joyyir changed the title feat: supports other charsets feat: PostResponse supports charset other than UTF-8 May 11, 2023
Copy link
Member

@nic-chen nic-chen left a comment

Choose a reason for hiding this comment

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

Thank you very much for the contribution, please add some test cases for the change.

@joyyir
Copy link
Contributor Author

joyyir commented May 15, 2023

Added test cases. Thank you.

@joyyir joyyir requested a review from nic-chen May 16, 2023 01:57
@@ -53,7 +57,7 @@ public ByteBuffer encode() {

int bodyIndex = -1;
if (StringUtils.hasText(body)) {
byte[] bodyBytes = body.getBytes(StandardCharsets.UTF_8);
byte[] bodyBytes = body.getBytes(charset);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
byte[] bodyBytes = body.getBytes(charset);
byte[] bodyBytes = body.getBytes(this.charset);

should be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reflected your review. Thank you.

nic-chen
nic-chen previously approved these changes May 16, 2023
Copy link
Member

@nic-chen nic-chen left a comment

Choose a reason for hiding this comment

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

LGTM

Gallardot
Gallardot previously approved these changes May 17, 2023
@joyyir joyyir dismissed stale reviews from Gallardot and nic-chen via 0aa3c9c May 22, 2023 02:37
@joyyir joyyir requested review from Gallardot and nic-chen May 22, 2023 02:41
@nic-chen nic-chen merged commit cdbd81f into apache:main May 22, 2023
2 of 3 checks passed
@nic-chen
Copy link
Member

@joyyir thanks!

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.

None yet

3 participants