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 line folding 2 #88

Closed
wants to merge 11 commits into from
Closed

Conversation

jonasfj
Copy link
Member

@jonasfj jonasfj commented Jun 12, 2024

@kekavc24 Í tried to play a bit with the things you did, and refactored a few things.

I'm pretty sure I also broke a lot of tests 🙈

Feel free to ignore or take inspiration.

kekavc24 and others added 11 commits June 6, 2024 13:42
> Fix invalid encoding for string with trailing line breaks or white-space
> Add a string test case in `string_tests.dart`
> Update `_tryYamlEncodedPlain` to return null if string cannot be encoded
> Make `_yamlEncodeFlowScalar` and `yamlEncodeBlockScalar` code concise
> Check for trailing whitespace rather than `\n `.
> `_yamlEncodeFlowScalar` and `_yamlEncodeBlockScalar` always encode YamlScalars
if (value.style == ScalarStyle.SINGLE_QUOTED) {
return _tryYamlEncodeSingleQuoted(val);
}
// TODO: Are there other strings we can't encode in folded mode?
Copy link
Member Author

Choose a reason for hiding this comment

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

There must be other things we can't represent in folded mode, right?

Copy link
Contributor

@kekavc24 kekavc24 Jun 17, 2024

Choose a reason for hiding this comment

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

The leading space was the main culprit.

Also the if statement for the leading space and line break (in view here) makes the tests fail 😅 I will carry on from your commit.

/// ```
///
/// See: https://yaml.org/spec/1.2.2/#813-folded-style
String? _tryYamlEncodeFolded(String string, int indentSize, String lineEnding) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Also I'm realizing that we might be missing the point with folded mode.

The point with folded mode is that we can replace a b with a\nb and still get the same value.

If the string contains newlines then this becomes complicated, because a newline essentially have to be replaced by an empty-line, right?

But also, rather importantly: the point of a folded string is to be able to wrap long lines without affecting the end-result.

A: |-
  my very long string that cannot be broken, if I don't want newlines.
# As folded string
B: >-
  my very long string that
  cannot be broken, if I
  don't want newlines.

In this case (A) and (B) are the same. The reason for using folded strings is that you don't want VERY long lines.

So if we want to support folded string, I guess we ought to maybe wrap strings at 80 characters, or something like that?

This is perhaps something we can do in a follow up PR, if we just make the folded string logic work correctly, then perhaps improving the formatting is a follow up.

Copy link
Contributor

@kekavc24 kekavc24 Jun 12, 2024

Choose a reason for hiding this comment

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

So if we want to support folded string, I guess we ought to maybe wrap strings at 80 characters, or something like that?

That's a fairly tricky (but achievable) implementation that's why I initially asked this in the comment. It seems I didn't frame it correctly. I meant it like,

"Should we leave it as-is with the formatting we do or mutate it to match yaml while still holding the same meaning" when parsed.

If the string contains newlines then this becomes complicated, because a newline essentially have to be replaced by an empty-line, right?

\n have to be \n\n in folded to hold the same meaning unless the next line starts with leading space then we can let it slide.

☝🏽 That makes it a bit hard to apply to the first line that starts with a leading space. Since at this point package: yaml won't have the indent information. That's why explicitly fall back to doubleQuoted. Will work on it next.

@jonasfj jonasfj mentioned this pull request Jun 13, 2024
1 task
Comment on lines 193 to 198
/// Simplest block style.
/// * https://yaml.org/spec/1.2.2/#812-literal-style
return '|${_getChompingIndicator(string)}\n$indent'
return '|${string.endsWith('\n') ? '' : '-'}\n$indent'
'${string.replaceAll('\n', lineEnding + indent)}';
}

Copy link
Contributor

@kekavc24 kekavc24 Jun 16, 2024

Choose a reason for hiding this comment

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

@jonasfj I finally got around to look into this. Why the change? Literal strings are also subject to chomping as indicated here

This causes anything with more than one trailing \n to fail as every new line that's empty will be clipped such that:

  1. foo bar \n remains as foo bar \n (okay)
  2. foo bar \n\n becomes foo bar \n (not okay).

That's why I went for a wildcard approach. We chomp so that it's always parsed correctly by package: yaml even if we only have a single line break.

@jonasfj
Copy link
Member Author

jonasfj commented Jun 27, 2024

Closing in favor of #87

@jonasfj jonasfj closed this Jun 27, 2024
@jonasfj jonasfj deleted the fix-line-folding-2 branch June 27, 2024 12:47
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.

2 participants