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

Escape single quotes in single-quoted string #256

Merged
merged 3 commits into from
Oct 16, 2021

Conversation

martin-sucha
Copy link
Contributor

According to YAML specification, single quotes must be repeated
in a single-quoted scalar.

https://yaml.org/spec/1.2.2/#732-single-quoted-style

Fixes #255

According to YAML specification, single quotes must be repeated
in a single-quoted scalar.

https://yaml.org/spec/1.2.2/#732-single-quoted-style

Fixes goccy#255
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2021

Codecov Report

Merging #256 (e5c9c3d) into master (bf7fe89) will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
+ Coverage   76.32%   76.44%   +0.12%     
==========================================
  Files          12       12              
  Lines        4194     4216      +22     
==========================================
+ Hits         3201     3223      +22     
  Misses        759      759              
  Partials      234      234              

ast/ast.go Outdated
// https://yaml.org/spec/1.2.2/#732-single-quoted-style
func escapeSingleQuote(s string) string {
var sb strings.Builder
sb.Grow(2+strings.Count(s, "'")*2)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
sb.Grow(2+strings.Count(s, "'")*2)
growLen := len(s)+2+strings.Count(s, "'")*2
sb.Grow(growLen)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting this!

As I'm reading the code again, the length should be sum of

  • len(s) for the original string (including ' characters)
  • 2 for the opening and closing '
  • strings.Count(s, "'") (without the *2) as each ' is already included in len(s) once

I've added a commit to fix the calculation.

Thanks goccy for review and suggestion.
@goccy
Copy link
Owner

goccy commented Oct 12, 2021

Thank you for adding test case to ast package .
But, there are currently no test cases for the ast package, which decreases coverage by more than 15%.
As a workaround, I suggest the following fix:

  • .codecov.yml
ignore:
  - ast

ast package did not have any tests previously and a new test
was added. The codecov tool computes that code coverage has dropped by
15% even though a new test was added.

Adding ast to ignore list as a workaround so that the pull request can
be merged.

Thanks goccy for the suggestion.
@martin-sucha
Copy link
Contributor Author

Added the codecov ignore. Thanks for the suggestion!

@goccy
Copy link
Owner

goccy commented Oct 16, 2021

Thank you for the contribution ! LGTM !

@goccy goccy merged commit c4f7968 into goccy:master Oct 16, 2021
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.

Single quoted string is not round-tripped correctly
3 participants