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.

Sign up for GitHub

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

Jump to bottom

Fix line folding 2 #88

Closed
wants to merge 11 commits into from
Closed

Fix line folding 2 #88

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f3886e6
Fix line folding to match specification
kekavc24 Jun 4, 2024
d46f402
Add test case and remove TODO for fix
kekavc24 Jun 4, 2024
c8c1695
Remove indent creation from utility function
kekavc24 Jun 6, 2024
7850c65
Add more string test cases
kekavc24 Jun 6, 2024
009c714
Update changelog
kekavc24 Jun 6, 2024
e047567
Return null if strings cannot be encoded in desired `ScalarStyle`
kekavc24 Jun 10, 2024
d94ba04
Add keep `+` chomping indicator tests
kekavc24 Jun 10, 2024
3837fec
Clean up code
kekavc24 Jun 11, 2024
c21c9ba
Fix keep `+` indicator condition
kekavc24 Jun 11, 2024
5c2f5c1
Remove unnecessary assertions and add docs
kekavc24 Jun 11, 2024
82a9796
Minor refactoring
jonasfj Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/src/editor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ class YamlEditor {
final end = getContentSensitiveEnd(_contents);
final lineEnding = getLineEnding(_yaml);
final edit = SourceEdit(
start, end - start, yamlEncodeBlockString(valueNode, 0, lineEnding));
start, end - start, yamlEncodeBlock(valueNode, 0, lineEnding));

return _performEdit(edit, path, valueNode);
}
Expand Down
10 changes: 5 additions & 5 deletions lib/src/list_mutations.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ SourceEdit updateInList(
final listIndentation = getListIndentation(yaml, list);
final indentation = listIndentation + getIndentation(yamlEdit);
final lineEnding = getLineEnding(yaml);
valueString = yamlEncodeBlockString(
wrapAsYamlNode(newValue), indentation, lineEnding);
valueString =
yamlEncodeBlock(wrapAsYamlNode(newValue), indentation, lineEnding);

/// We prefer the compact nested notation for collections.
///
Expand All @@ -52,7 +52,7 @@ SourceEdit updateInList(

return SourceEdit(offset, end - offset, valueString);
} else {
valueString = yamlEncodeFlowString(newValue);
valueString = yamlEncodeFlow(newValue);
return SourceEdit(offset, currValue.span.length, valueString);
}
}
Expand Down Expand Up @@ -141,7 +141,7 @@ SourceEdit _appendToBlockList(
final newIndentation = listIndentation + getIndentation(yamlEdit);
final lineEnding = getLineEnding(yaml);

var valueString = yamlEncodeBlockString(item, newIndentation, lineEnding);
var valueString = yamlEncodeBlock(item, newIndentation, lineEnding);
if (isCollection(item) && !isFlowYamlCollectionNode(item) && !isEmpty(item)) {
valueString = valueString.substring(newIndentation);
}
Expand All @@ -151,7 +151,7 @@ SourceEdit _appendToBlockList(

/// Formats [item] into a new node for flow lists.
String _formatNewFlow(YamlList list, YamlNode item, [bool isLast = false]) {
var valueString = yamlEncodeFlowString(item);
var valueString = yamlEncodeFlow(item);
if (list.isNotEmpty) {
if (isLast) {
valueString = ', $valueString';
Expand Down
14 changes: 7 additions & 7 deletions lib/src/map_mutations.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ SourceEdit _addToBlockMap(
final yaml = yamlEdit.toString();
final newIndentation =
getMapIndentation(yaml, map) + getIndentation(yamlEdit);
final keyString = yamlEncodeFlowString(wrapAsYamlNode(key));
final keyString = yamlEncodeFlow(wrapAsYamlNode(key));
final lineEnding = getLineEnding(yaml);

var formattedValue = ' ' * getMapIndentation(yaml, map);
Expand Down Expand Up @@ -83,7 +83,7 @@ SourceEdit _addToBlockMap(
}
}

var valueString = yamlEncodeBlockString(newValue, newIndentation, lineEnding);
var valueString = yamlEncodeBlock(newValue, newIndentation, lineEnding);
if (isCollection(newValue) &&
!isFlowYamlCollectionNode(newValue) &&
!isEmpty(newValue)) {
Expand All @@ -100,8 +100,8 @@ SourceEdit _addToBlockMap(
/// map.
SourceEdit _addToFlowMap(
YamlEditor yamlEdit, YamlMap map, YamlNode keyNode, YamlNode newValue) {
final keyString = yamlEncodeFlowString(keyNode);
final valueString = yamlEncodeFlowString(newValue);
final keyString = yamlEncodeFlow(keyNode);
final valueString = yamlEncodeFlow(newValue);

// The -1 accounts for the closing bracket.
if (map.isEmpty) {
Expand Down Expand Up @@ -131,8 +131,8 @@ SourceEdit _replaceInBlockMap(
getMapIndentation(yaml, map) + getIndentation(yamlEdit);

final keyNode = getKeyNode(map, key);
var valueAsString = yamlEncodeBlockString(
wrapAsYamlNode(newValue), newIndentation, lineEnding);
var valueAsString =
yamlEncodeBlock(wrapAsYamlNode(newValue), newIndentation, lineEnding);
if (isCollection(newValue) &&
!isFlowYamlCollectionNode(newValue) &&
!isEmpty(newValue)) {
Expand Down Expand Up @@ -163,7 +163,7 @@ SourceEdit _replaceInBlockMap(
SourceEdit _replaceInFlowMap(
YamlEditor yamlEdit, YamlMap map, Object? key, YamlNode newValue) {
final valueSpan = map.nodes[key]!.span;
final valueString = yamlEncodeFlowString(newValue);
final valueString = yamlEncodeFlow(newValue);

return SourceEdit(valueSpan.start.offset, valueSpan.length, valueString);
}
Expand Down
152 changes: 65 additions & 87 deletions lib/src/strings.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,32 +8,21 @@ import 'package:yaml/yaml.dart';
import 'utils.dart';

/// Given [value], tries to format it into a plain string recognizable by YAML.
/// If it fails, it defaults to returning a double-quoted string.
///
/// Not all values can be formatted into a plain string. If the string contains
/// an escape sequence, it can only be detected when in a double-quoted
/// sequence. Plain strings may also be misinterpreted by the YAML parser (e.g.
/// ' null').
String? _tryYamlEncodePlain(Object? value) {
if (value is YamlNode) {
AssertionError(
'YamlNodes should not be passed directly into getSafeString!',
);
}

assertValidScalar(value);

if (value is String) {
/// If it contains a dangerous character we want to wrap the result with
/// double quotes because the double quoted style allows for arbitrary
/// strings with "\" escape sequences.
///
/// See 7.3.1 Double-Quoted Style
/// https://yaml.org/spec/1.2/spec.html#id2787109
return isDangerousString(value) ? null : value;
}

return value.toString();
///
/// Returns `null` if [value] cannot be encoded as a plain string.
String? _tryYamlEncodePlain(String value) {
/// If it contains a dangerous character we want to wrap the result with
/// double quotes because the double quoted style allows for arbitrary
/// strings with "\" escape sequences.
///
/// See 7.3.1 Double-Quoted Style
/// https://yaml.org/spec/1.2/spec.html#id2787109
return isDangerousString(value) ? null : value;
}

/// Checks if [string] has unprintable characters according to
Expand All @@ -48,41 +37,6 @@ bool _hasUnprintableCharacters(String string) {
return false;
}

/// Checks if a [string] has any unprintable characters or characters that
/// should be explicitly wrapped in double quotes according to
/// [unprintableCharCodes] and [doubleQuoteEscapeChars] respectively.
///
/// It should be noted that this check excludes the `\n` (line break)
/// character as it is encoded correctly when using [ScalarStyle.LITERAL] or
/// [ScalarStyle.FOLDED].
bool _shouldDoubleQuote(String string) {
if (string.isEmpty || string.trimLeft().length != string.length) return true;

final codeUnits = string.codeUnits;

return doubleQuoteEscapeChars.keys
.whereNot((charUnit) => charUnit == 10) // Anything but line breaks
.any(codeUnits.contains);
}

/// Returns the correct block chomping indicator for [ScalarStyle.FOLDED]
/// and [ScalarStyle.LITERAL].
///
/// See https://yaml.org/spec/1.2.2/#8112-block-chomping-indicator
String _getChompingIndicator(String string) {
/// By default, we apply an indent to the string after every new line.
///
/// Apply the `keep (+)` chomping indicator for trailing whitespace to be
/// treated as content.
///
/// [NOTE]: We only check for trailing whitespace rather than `\n `. This is
/// a coin-toss approach. If there is a new line after this, it will be kept.
/// If not, nothing happens.
if (string.endsWith(' ') || string.endsWith('\n')) return '+';

return '-';
}

/// Generates a YAML-safe double-quoted string based on [string], escaping the
/// list of characters as defined by the YAML 1.2 spec.
///
Expand Down Expand Up @@ -119,6 +73,24 @@ String? _tryYamlEncodeSingleQuoted(String string) {
return '\'$result\'';
}

/// Returns the correct block chomping indicator for [ScalarStyle.FOLDED]
/// and [ScalarStyle.LITERAL].
///
/// See https://yaml.org/spec/1.2.2/#8112-block-chomping-indicator
String _getChompingIndicator(String string) {
/// By default, we apply an indent to the string after every new line.
///
/// Apply the `keep (+)` chomping indicator for trailing whitespace to be
/// treated as content.
///
/// [NOTE]: We only check for trailing whitespace rather than `\n `. This is
/// a coin-toss approach. If there is a new line after this, it will be kept.
/// If not, nothing happens.
if (string.endsWith(' ') || string.endsWith('\n')) return '+';

return '-';
}

/// Attempts to encode a [string] as a _YAML folded string_ and apply the
/// appropriate _chomping indicator_.
///
Expand All @@ -140,7 +112,12 @@ String? _tryYamlEncodeSingleQuoted(String string) {
///
/// 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 •
edited
Loading

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.

if (_shouldDoubleQuote(string)) return null;
if (_hasUnprintableCharacters(string)) return null;
// A string that starts with space or newline followed by space can't be
// encoded in folded mode.
if (string.startsWith(' ') && string.startsWith('\n ')) return null;

// 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 •
edited
Loading

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.


final indent = ' ' * indentSize;

Expand Down Expand Up @@ -195,22 +172,27 @@ String? _tryYamlEncodeFolded(String string, int indentSize, String lineEnding) {
/// my literal
/// string
///
/// # With the "keep" chomping indicator
/// key: |+
/// # Without chomping indicator
/// key: |
/// my literal
/// string
/// ```
///
/// See: https://yaml.org/spec/1.2.2/#812-literal-style
String? _tryYamlEncodeLiteral(
String string, int indentSize, String lineEnding) {
if (_shouldDoubleQuote(string)) return null;
if (_hasUnprintableCharacters(string)) return null;
// A string that starts with space or newline followed by space can't be
// encoded in literal mode.
if (string.startsWith(' ') && string.startsWith('\n ')) return null;

// TODO: Are there other strings we can't encode in literal mode?

final indent = ' ' * indentSize;

/// 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 •
edited
Loading

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.

Expand All @@ -222,22 +204,23 @@ String? _tryYamlEncodeLiteral(
String _yamlEncodeFlowScalar(YamlScalar yamlScalar) {
final YamlScalar(:value, :style) = yamlScalar;

final isString = value is String;
if (value is! String) {
return value.toString();
}

switch (style) {
/// Only encode as double-quoted if it's a string.
case ScalarStyle.DOUBLE_QUOTED when isString:
case ScalarStyle.DOUBLE_QUOTED:
return _yamlEncodeDoubleQuoted(value);

case ScalarStyle.SINGLE_QUOTED when isString:
case ScalarStyle.SINGLE_QUOTED:
return _tryYamlEncodeSingleQuoted(value) ??
_yamlEncodeDoubleQuoted(value);

/// Cast into [String] if [null] as this condition only returns [null]
/// for a [String] that can't be encoded.
default:
return _tryYamlEncodePlain(value) ??
_yamlEncodeDoubleQuoted(value as String);
return _tryYamlEncodePlain(value) ?? _yamlEncodeDoubleQuoted(value);
}
}

Expand All @@ -253,41 +236,37 @@ String _yamlEncodeBlockScalar(
final YamlScalar(:value, :style) = yamlScalar;
assertValidScalar(value);

final isString = value is String;

if (isString && _hasUnprintableCharacters(value)) {
return _yamlEncodeDoubleQuoted(value);
if (value is! String) {
return value.toString();
}

switch (style) {
/// Prefer 'plain', fallback to "double quoted". Cast into [String] if
/// null as this condition only returns [null] for a [String] that can't
/// be encoded.
case ScalarStyle.PLAIN:
return _tryYamlEncodePlain(value) ??
_yamlEncodeDoubleQuoted(value as String);
return _tryYamlEncodePlain(value) ?? _yamlEncodeDoubleQuoted(value);

// Prefer 'single quoted', fallback to "double quoted"
case ScalarStyle.SINGLE_QUOTED when isString:
case ScalarStyle.SINGLE_QUOTED:
return _tryYamlEncodeSingleQuoted(value) ??
_yamlEncodeDoubleQuoted(value);

/// Prefer folded string, try literal as fallback
/// otherwise fallback to "double quoted"
case ScalarStyle.FOLDED when isString:
case ScalarStyle.FOLDED:
return _tryYamlEncodeFolded(value, indentation, lineEnding) ??
_yamlEncodeDoubleQuoted(value);

/// Prefer literal string, try folded as fallback
/// otherwise fallback to "double quoted"
case ScalarStyle.LITERAL when isString:
case ScalarStyle.LITERAL:
return _tryYamlEncodeLiteral(value, indentation, lineEnding) ??
_yamlEncodeDoubleQuoted(value);

/// Prefer plain, fallback to "double quoted"
default:
return _tryYamlEncodePlain(value) ??
_yamlEncodeDoubleQuoted(value as String);
return _tryYamlEncodePlain(value) ?? _yamlEncodeDoubleQuoted(value);
}
}

Expand All @@ -298,16 +277,16 @@ String _yamlEncodeBlockScalar(
/// string scalar that starts with '>', a child having a block style
/// parameters), in which case we will produce [value] with default styling
/// options.
String yamlEncodeFlowString(YamlNode value) {
String yamlEncodeFlow(YamlNode value) {
if (value is YamlList) {
final list = value.nodes;

final safeValues = list.map(yamlEncodeFlowString);
final safeValues = list.map(yamlEncodeFlow);
return '[${safeValues.join(', ')}]';
} else if (value is YamlMap) {
final safeEntries = value.nodes.entries.map((entry) {
final safeKey = yamlEncodeFlowString(entry.key as YamlNode);
final safeValue = yamlEncodeFlowString(entry.value);
final safeKey = yamlEncodeFlow(entry.key as YamlNode);
final safeValue = yamlEncodeFlow(entry.value);
return '$safeKey: $safeValue';
});

Expand All @@ -318,14 +297,14 @@ String yamlEncodeFlowString(YamlNode value) {
}

/// Returns [value] with the necessary formatting applied in a block context.
String yamlEncodeBlockString(
String yamlEncodeBlock(
YamlNode value,
int indentation,
String lineEnding,
) {
const additionalIndentation = 2;

if (!isBlockNode(value)) return yamlEncodeFlowString(value);
if (!isBlockNode(value)) return yamlEncodeFlow(value);

final newIndentation = indentation + additionalIndentation;

Expand All @@ -337,8 +316,7 @@ String yamlEncodeBlockString(
final children = value.nodes;

safeValues = children.map((child) {
var valueString =
yamlEncodeBlockString(child, newIndentation, lineEnding);
var valueString = yamlEncodeBlock(child, newIndentation, lineEnding);
if (isCollection(child) && !isFlowYamlCollectionNode(child)) {
valueString = valueString.substring(newIndentation);
}
Expand All @@ -353,10 +331,10 @@ String yamlEncodeBlockString(
return value.nodes.entries.map((entry) {
final MapEntry(:key, :value) = entry;

final safeKey = yamlEncodeFlowString(key as YamlNode);
final safeKey = yamlEncodeFlow(key as YamlNode);
final formattedKey = ' ' * indentation + safeKey;

final formattedValue = yamlEncodeBlockString(
final formattedValue = yamlEncodeBlock(
value,
newIndentation,
lineEnding,
Expand Down
Loading