Fix splice list insertion #84

kekavc24 · 2024-05-31T11:22:46Z

This PR:

Closes Splice list in a nested block list fails! #83
Fixes the indent calculation for block lists
Adds golden tests for spliceList method

@jonasfj Everything works fine and as expected. However, there is a test in randomTest.dart that fails because the string generated is invalid and is never parsed correctly from the start.

It's the only test failing.

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

jonasfj · 2024-05-31T12:42:05Z

test/testdata/input/splicelist_in_nested_block_list.test

+- true
+- - false
+---
+- [splice, [key, 1], 0, 1, ['test']]


Missing newline at end of the line.

Consider making a configuration change to your editor.

jonasfj · 2024-05-31T14:44:30Z

@jonasfj Everything works fine and as expected. However, there is a test in randomTest.dart that fails because the string generated is invalid and is never parsed correctly from the start.

Can we change it such that it doesn't generate invalid cases?

We need to either skip or fix tests, if they are unrelated to this issue, I don't mind skipping and filing an issue to fix them separately.

test/testdata/input/splicelist_in_nested_block_list.test

kekavc24 · 2024-05-31T15:38:09Z

@jonasfj Everything works fine and as expected. However, there is a test in randomTest.dart that fails because the string generated is invalid and is never parsed correctly from the start.

Can we change it such that it doesn't generate invalid cases?

We need to either skip or fix tests, if they are unrelated to this issue, I don't mind skipping and filing an issue to fix them separately.

It's the 20th iteration. Same random string (doesn't feel random) each time. An issue will be fine. I've skipped it for now. I can tackle it separately. I had to modify the Generator class slightly to get the current iteration.

jonasfj

nits only really :D

Looks like a solid solution to me.

jonasfj · 2024-06-03T12:05:49Z

lib/src/list_mutations.dart

+  /// Anytime our '-' is non-existent, use '\n' as source of truth. Also, if
+  /// the new line is closer


What does this mean, when does this happen? Can it happen?

Suggested change

/// Anytime our '-' is non-existent, use '\n' as source of truth. Also, if

/// the new line is closer

/// If '\n' comes before '-' then we're not inside a nested block list.

Yeah, it does since we are scanning backwards. I think you covered it in the last comment. Basically if:

Index of - is -1 or less than \n index (even if \n index is -1), we always assume we inserting it i.e. +1.

Otherwise, we insert after the last - i.e. `+2 inclusive of space after if nested in list.

nit: Should we update the comment or not?

I'll make it clearer.

test/testdata/input/splicelist_in_nested_block_list.test

jonasfj · 2024-06-03T12:16:04Z

lib/src/list_mutations.dart

+/// Checks if the [YamlNode]'s list is directly nested within another list
+(bool isNested, int offset) _isNestedInBlockList(int start, String yaml) {


Are we sure it's a good idea to make this a function? are we going to use it elsewhere?

I don't mind, but if we do, we probably should document it clearly.

Like what is start what is yaml and what is the list being talked about.

The documentation has to explain that start is expected to be the offsets of the - indicator for an element in a list formatting in block-mode, and yaml is the entire YAML document.

And this method aims to find out if:

The element that has a start offset at start is inside a nested block list

offset for...

Since this will probably only be used inside _insertInBlockList we could consider inlining it. Also given how hard it is to explain what start really is.

But add documentation comments that explains what it does is also fine.

I extracted it to make _insertInBlockList readable. Even though the function is doing a relatively simple thing, it can be hard to follow if leave it inside due to the many yaml.lastIndeOf calls we make

jonasfj · 2024-06-03T12:36:54Z

lib/src/list_mutations.dart

-  final start = yaml.lastIndexOf('\n', currNodeStart) + 1;

-  return SourceEdit(start, 0, formattedValue);
+  var currSequenceOffset = yaml.lastIndexOf('-', currNodeStart - 1);
+
+  final (isNested, offset) = _isNestedInBlockList(currSequenceOffset - 1, yaml);
+
+  /// We have to get rid of the left indentation applied by default
+  if (isNested && index == 0) {
+    // Give the previous first element its indent
+    formattedValue = formattedValue.trimLeft() + indent;
+  }
+
+  return SourceEdit(offset, 0, formattedValue);
+}


So when we're inserting into a block list, we:

Find the node that is currently at the index we want to insert at.

Find the - indicator in front of that node.

Determine if we're in a nested block list or not

If so, ...?

else we....

I think what you're doing is probably correct in both cases, it's just a bit weird.

If I have a nested list:

- - foo

and I'm prepending bar to the inner list, then I get a diff that looks like:

- - foo ^ Insert: "- bar\n "

Where as if I have:

- foo

and I prepend bar I get a diff that says:

- foo ^ Insert: " - bar\n"

So in the case with no nested lists, we're inserting a full new line.
The - indicator that belongs to foo moves down.

Where as, if we have nested block lists, we insert into the middle of the existing line.

I guess this makes sense because in - - the first - belongs to the outer list, and we're not modifying the outer list. We're modifying the inner list.

Just typing this out so I think I understand it, this seems solid.

I'd suggest we either inline _isNestedInBlockList or improve the documentation so that it can stand on its own -- otherwise, it's a bit hard to understand what it does when someone reads it.

I guess technically we could in the second case have done:

- foo ^ Insert: "- bar\n "

That would be more consistent with the first case, and we wouldn't need to know if it's a nested list or not. But the diff is much better when we're inserting a full line, and that's also the common case after all, nested lists in block mode is fairly weird. So I totally understand why we need to know if it's inside a nested block list.

😅 yaml syntax is quirky making everything weird. The existing code works well and assumes people will be civil and use

- - value

instead of

- - value

oh, I didn't even realize that:

- - value

was an option... can we add a few tests 🤣

oh, I didn't even realize that:

- - value

was an option... can we add a few tests 🤣

Added these in 5978c20, I think.

Added additional docs

jonasfj

just nits

test/testdata/input/splice_list_in_nested_block_with_weird_spaces.test

jonasfj · 2024-06-04T14:30:57Z

lib/src/list_mutations.dart

@@ -132,7 +132,8 @@ SourceEdit _appendToBlockList(
 }

 /// Formats [item] into a new node for block lists.
-String _formatNewBlock(YamlEditor yamlEdit, YamlList list, YamlNode item) {
+(String formatted, String indent) _formatNewBlock(


Are you sure it wouldn't make more sense to return listIndentation ? Instead of returning the actual string.

I'll investigate that. I did that for convenience. I didn't want to change so many things in the existing code.

jonasfj · 2024-06-04T14:35:25Z

lib/src/list_mutations.dart

+    /// Since we CANT'T/SHOULDN'T manipulate the next element to get rid of the
+    /// space it has, we remove the padding (if any is present) from the indent
+    /// itself.
+    indent = indent.replaceFirst(padding, '');


Couldn't we use indent.substring()...

But also if we made _formatNewBlock return listIndentation instead of a string, then we can just do ' ' * (listIndentation - leftPad), right?

Or am I wrong here?

I'm just wondering, if maybe it's smarter to return integers instead of strings, when we want to communicate indentation. We return an integer from getListIndentation.

Honestly, we could probably also just call getListIndentation(yaml, list); again, rather than change _formatNewBlock, but I don't mind returning both from _formatNewBlock :D

This actually makes sense. The _formatNewBlock can be tweaked and the other reference updated.

test/testdata/input/splicelist_in_nested_block_list.test

…ces.test

jonasfj · 2024-06-06T10:29:19Z

@kekavc24 by the way, feel free to file a PR with a new bullet point for the CHANGELOG.md. it might be nice for package consumers to know what we fixed :D

…annel, webdev, yaml_edit Revisions updated by `dart tools/rev_sdk_deps.dart`. crypto (https://github.com/dart-lang/crypto/compare/0a10171..7a9428a): 7a9428a 2024-06-04 Kevin Moore Bump lints dep (dart-archive/crypto#172) dartdoc (https://github.com/dart-lang/dartdoc/compare/45627f9..ddb8fb4): ddb8fb44 2024-06-03 Konstantin Scheglov Use package:analyzer/source/source.dart (dart-lang/dartdoc#3780) http (https://github.com/dart-lang/http/compare/7bfbeea..a3567f8): a3567f8 2024-06-05 Brian Quinlan Prepare to release cronet_http 1.3.1 (dart-lang/http#1228) 7addc61 2024-06-05 Hossein Yousefi [cronet_http] Upgrade jnigen to 0.9.2 to close `#1213` (dart-lang/http#1225) eb87a60 2024-06-04 Anikate De pkgs/ok_http: Close the client (override `close()` from `BaseClient`) (dart-lang/http#1224) 9f022d2 2024-06-04 Anikate De pkgs/http_client_conformance_tests: add boolean flag `supportsFoldedHeaders` (dart-lang/http#1222) 90837df 2024-06-03 Anikate De pkgs/ok_http: Condense JNI Bindings to `single_file` structure, and add missing server errors test (dart-lang/http#1221) logging (https://github.com/dart-lang/logging/compare/73f043a..dbd6829): dbd6829 2024-06-04 Sarah Zakarias Update `topics` in `pubspec.yaml` (dart-archive/logging#165) test (https://github.com/dart-lang/test/compare/2464ad5..83c597e): 83c597e5 2024-06-05 Jacob MacDonald enable asserts in dart2wasm tests (dart-lang/test#2241) 60353804 2024-06-04 Nate Bosch Remove an unnecessary `dynamic` (dart-lang/test#2239) 43ad4cd8 2024-06-03 Kevin Moore random cleanup (dart-lang/test#2232) 18db77c3 2024-06-02 Kevin Moore prepare to publish test_api (dart-lang/test#2238) cd72e8d8 2024-06-02 Kevin Moore Prepare to publish (dart-lang/test#2237) 9c6adaa7 2024-06-01 Kevin Moore fixes (dart-lang/test#2235) 0110a80b 2024-06-01 dependabot[bot] Bump the github-actions group with 2 updates (dart-lang/test#2236) b1b2c029 2024-06-01 Martin Kustermann Fix `dart run test -p chrome -c dart2wasm` (dart-lang/test#2233) tools (https://github.com/dart-lang/tools/compare/86b3661..4321aec): 4321aec 2024-06-05 Andrew Kolos [unified_analytics] Suppress any `FileSystemException` thrown during `Analytics.send` (dart-lang/tools#274) e5d4c8b 2024-06-03 dependabot[bot] Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/tools#271) web_socket_channel (https://github.com/dart-lang/web_socket_channel/compare/45b8ce9..bf69990): bf69990 2024-06-03 Thibault Chatillon bump web_socket dependency 0.1.3 -> 0.1.5 (dart-lang/web_socket_channel#370) 5b428dd 2024-06-02 dependabot[bot] Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/web_socket_channel#371) afd1e3c 2024-05-23 Brian Quinlan Remove `--fatal-infos` from `dart pub downgrade` analysis (dart-lang/web_socket_channel#367) cb20b71 2024-05-23 Sarah Zakarias Add `topics` to `pubspec.yaml` (dart-lang/web_socket_channel#362) 8514229 2024-05-22 Kevin Moore Bump and fix lints (dart-lang/web_socket_channel#366) webdev (https://github.com/dart-lang/webdev/compare/a97c2a1..9ada46f): 9ada46fc 2024-06-05 Nicholas Shahan Hide more temporary variables when debugging (dart-lang/webdev#2445) yaml_edit (https://github.com/dart-lang/yaml_edit/compare/963e7a3..08a146e): 08a146e 2024-06-06 Kavisi Fix splice list insertion (dart-lang/yaml_edit#84) 561309e 2024-06-01 dependabot[bot] Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/yaml_edit#85) b582fd5 2024-05-31 Kavisi Fix error thrown when inserting keys (dart-lang/yaml_edit#80) Change-Id: I36acbc26fe97d8a1e3fdfa5f4855cc4be7d84dd7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/370080 Commit-Queue: Devon Carew <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]>

kekavc24 added 4 commits May 31, 2024 12:01

Fix spliceList error for nested lists

76df4d8

Fix list indentation calculation

94c1d3b

Add golden tests for spliceList

905f77e

Remove skip flag on test added in 8cc8580

fe44d09

jonasfj reviewed May 31, 2024

View reviewed changes

kekavc24 added 2 commits May 31, 2024 14:51

Add missing line at end of file

5f369b4

Add spliceList tests

5978c20

jonasfj reviewed May 31, 2024

View reviewed changes

test/testdata/input/splicelist_in_nested_block_list.test Outdated Show resolved Hide resolved

Skip random test that fails (20th iteration)

f223d48

Add deeply nested input in golden tests for spliceList

825b7cf

kekavc24 requested a review from jonasfj May 31, 2024 15:55

jonasfj approved these changes Jun 3, 2024

View reviewed changes

kekavc24 added 3 commits June 4, 2024 07:32

Fix formatting to account for space in nested list

436a2b5

Add golden tests for weird spaces in nested list

1b0c434

Run dart format

5e71dfb

kekavc24 requested a review from jonasfj June 4, 2024 06:39

kekavc24 mentioned this pull request Jun 4, 2024

Fix line folding #87

Merged

1 task

jonasfj approved these changes Jun 4, 2024

View reviewed changes

Update _formatNewBlock to return indent size instead of the indent

9c735e5

kekavc24 requested a review from jonasfj June 6, 2024 08:59

Update test/testdata/input/splice_list_in_nested_block_with_weird_spa…

66b25a9

…ces.test

jonasfj approved these changes Jun 6, 2024

View reviewed changes

jonasfj merged commit 08a146e into dart-lang:main Jun 6, 2024
10 checks passed

kekavc24 deleted the fix-splice-list-insertion branch June 6, 2024 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix splice list insertion #84

Fix splice list insertion #84

kekavc24 commented May 31, 2024

jonasfj May 31, 2024

jonasfj commented May 31, 2024

kekavc24 commented May 31, 2024 •

edited

Loading

jonasfj left a comment

jonasfj Jun 3, 2024

kekavc24 Jun 3, 2024

jonasfj Jun 3, 2024

kekavc24 Jun 4, 2024

jonasfj Jun 3, 2024

jonasfj Jun 3, 2024

kekavc24 Jun 3, 2024

jonasfj Jun 3, 2024

kekavc24 Jun 3, 2024

jonasfj Jun 3, 2024

kekavc24 Jun 3, 2024

kekavc24 Jun 4, 2024

jonasfj left a comment

jonasfj Jun 4, 2024

kekavc24 Jun 4, 2024 •

edited

Loading

jonasfj Jun 4, 2024

kekavc24 Jun 4, 2024

jonasfj commented Jun 6, 2024

		/// Anytime our '-' is non-existent, use '\n' as source of truth. Also, if
		/// the new line is closer

	/// Anytime our '-' is non-existent, use '\n' as source of truth. Also, if
	/// the new line is closer
	/// If '\n' comes before '-' then we're not inside a nested block list.

		/// Checks if the [YamlNode]'s list is directly nested within another list
		(bool isNested, int offset) _isNestedInBlockList(int start, String yaml) {

Fix splice list insertion #84

Fix splice list insertion #84

Conversation

kekavc24 commented May 31, 2024

Choose a reason for hiding this comment

jonasfj commented May 31, 2024

kekavc24 commented May 31, 2024 • edited Loading

jonasfj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasfj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kekavc24 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasfj commented Jun 6, 2024

kekavc24 commented May 31, 2024 •

edited

Loading

kekavc24 Jun 4, 2024 •

edited

Loading