Skip to content
This repository has been archived by the owner on Mar 31, 2023. It is now read-only.

use strings.ToValidUTF8 to validate string #26

Merged
merged 5 commits into from
Mar 19, 2020

Conversation

nokute78
Copy link
Contributor

ref #19

The patch replaces invalid utf8 string with whitespace.
e.g.
hello\x80world -> hello world

I just modified only plain*.go. Could you review it ?
If the review is ok, I will modify json*.go and logfmt*.go.

@ymmt2005
Copy link
Member

@nokute78
Thank you for the PR!

I'd like to use utf8.RuneError character instead of the white space.
Could you update the PR?

@nokute78
Copy link
Contributor Author

@ymmt2005 Thank you for quick reviewing.

I updated the PR.

@ymmt2005
Copy link
Member

Looks good!

Please update json and logfmt the samely.

@nokute78
Copy link
Contributor Author

@ymmt2005 Thank you for reviewing.
I modified json/logfmt files.

Note: json_test.go already tests invalid utf8 string, so I didn't modify it.

ymmt2005
ymmt2005 previously approved these changes Mar 18, 2020
Copy link
Member

@ymmt2005 ymmt2005 left a comment

Choose a reason for hiding this comment

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

LGTM.

@Hsn723 Could you take another look?

@Hsn723
Copy link
Member

Hsn723 commented Mar 18, 2020

@nokute78
Please add the go 1.13 directive to go.mod as strings.ToValidUTF8 can only be used starting go 1.13. Other than that, looks good.

@ymmt2005
After merging these changes, I'll update the Readme to indicate that go 1.13 or greater is required and cut a new v1.6.0 release.

@nokute78
Copy link
Contributor Author

@Hsn723 @ymmt2005 Thank you for reviewing.
I added go directive.

Copy link
Member

@Hsn723 Hsn723 left a comment

Choose a reason for hiding this comment

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

LGTM
@nokute78 Thank you for your contribution.

@Hsn723 Hsn723 merged commit 6ee3fcd into cybozu-go:master Mar 19, 2020
@nokute78 nokute78 deleted the issue_19 branch March 20, 2020 00:25
@nokute78 nokute78 mentioned this pull request Mar 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants