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

Fix indentation for raw string newlines #625

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Conversation

shuheiktgw
Copy link
Collaborator

Closes #292

Before submitting your PR, please confirm the following.

  • Describe the purpose for which you created this PR.
  • Create test code that corresponds to the modification

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.36%. Comparing base (f739772) to head (c559864).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #625   +/-   ##
=======================================
  Coverage   77.36%   77.36%           
=======================================
  Files          22       22           
  Lines        7885     7887    +2     
=======================================
+ Hits         6100     6102    +2     
  Misses       1368     1368           
  Partials      417      417           

@@ -28,8 +28,6 @@ const (
type Encoder struct {
writer io.Writer
opts []EncodeOption
indent int
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Encoder previously had two fields, indent and indentNum, but indentNum was not being used anywhere. Since the name indentNum is used elsewhere, I migrated the functionality of the indent field to the indentNum field and removed the indent field.

@shuheiktgw shuheiktgw requested a review from goccy January 22, 2025 08:19
if e.indentSequence {
e.column -= e.indentNum
}
}()
Copy link
Collaborator Author

@shuheiktgw shuheiktgw Jan 22, 2025

Choose a reason for hiding this comment

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

Minor refactoring to ensure the subtraction 🙏

@@ -816,11 +816,12 @@ func (n *StringNode) String() string {
// It works mostly, but inconsistencies occur if line break characters are mixed.
header := token.LiteralBlockHeader(n.Value)
space := strings.Repeat(" ", n.Token.Position.Column-1)
indent := strings.Repeat(" ", n.Token.Position.IndentNum)
Copy link
Owner

Choose a reason for hiding this comment

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

The token's IndentNum represents the number of spaces from the beginning of the line. In other words, this is not a value that represents "how many spaces are used for indentation".

Therefore, the behavior is different between the case where the Go value is directly converted to a string using the encoder, and the case where the string is created using the ast obtained by parsing.

However, when encoding the value of Go directly, the context literal is missing, so the AST that is always created becomes a StringNode, whereas when parsing, it becomes a LiteralNode. For this reason, treating the value of IndentNum in this way works well for StringNode.

This behavior may cause confusion, so it may be necessary to refactor it in the future.

@goccy
Copy link
Owner

goccy commented Jan 31, 2025

LGTM 👍

@goccy goccy merged commit c8cc5c5 into master Jan 31, 2025
20 checks passed
@goccy goccy deleted the fix_raw_string_indent branch January 31, 2025 01:27
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.

Block scalars does not follow indentation
3 participants