Prevent `YamlEditor.remove` from affecting existing block scalars #2240

kekavc24 · 2025-11-07T11:58:13Z

Issue(s)

Closes Comment causes issue with ScalarStyle.FOLDED #1935 and possibly lessens the work needed for Literal and folded strings cannot handle "\n \n" at the end of a string #1944

Changes

Calling YamlEditor.remove on a block map/list now removes the entire node completely and leaves no dangling comments or line breaks.
_removeFromBlockMap and _removeFromBlockList have been refactored to have shared logic. The heuristics for performing their edits are quite similar.

Disclaimers

This is a positive but breaking change. I have not changed the failing tests to showcase difference in output once/if this change lands.

Rationale

While YamlEditor itself doesn't support inserting ScalarStyle.FOLDED and ScalarStyle.LITERAL strings with trailing line breaks, there is a probability (even if it's low) that it might get a yaml string with these styles. It would be nice if it can handle this.

It uses a "if-not-first-then-discard-all" approach. It scans around the node and checks the next node (if present) for context before performing the edit. Looking ahead is easier than backtracking. See examples.

Case 1: Top level list

--- # Before edit
- value
- >+
  folded keep chomp

- removed # Funky comment
          # More indented
            # Messes with literal & folded.
- next

--- # After edit. Block scalar is safe
- value
- >+
  folded keep chomp

- next

Case 2: Top level maps

--- # Before edit
key: value
block: >+
  folded keep chomp

removed: entry # Funky comment
                    # More indented
                      # Messes with literal & folded.
next: entry

--- # After edit. Block scalar is safe
key: value
block: >+
  folded keep chomp

next: entry

Case 3: Compact lists

Same logic applies to block maps.

Variant 1

--- # Before edit

- - removed # My comment
  - next

--- # After edit

- - next

Variant 2 - Lenient when making nodes compact. This is the first node. We have the next as the buffer.

--- # Before edit

- - removed # Comment
            # No man's land
  - next

--- # After edit

-          # No man's land
  - next

Variant 3 - Strict. Who is before? Can check (is it worth it?), truncates instead.

--- # Before edit

- - |+
      literal keep chomp

  - removed # Comment
            # No man's land
  - next

--- # After edit

- - |+
      literal keep chomp

  - next

Variant 4 - Okay. Who is before? Can check (*BUT* is it worth it?), truncates instead.

--- # Before edit

- - Who is here

# Who? me?
# Just hanging around
# https://youtube.com/shorts/GPIwgAUwXYw?si=B8RzE6B0uXJQWHk-

  - removed # Comment
            # No man's land

  - next

--- # After edit

- - Who is here

# Who? me?
# Just hanging around
# https://youtube.com/shorts/GPIwgAUwXYw?si=B8RzE6B0uXJQWHk-

  - next

Variant 5 - Trailing. Removed.

--- # Before edit
- value
- another
- removed # Funky comment
          # More indented
            # Messes with literal & folded.

--- # After edit. Block scalar is safe
- value
- another

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.

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

Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.

gemini-code-assist

Code Review

This pull request introduces a significant and well-reasoned refactoring to how YAML block collection entries are removed, aiming to prevent unintended effects on block scalars. The new removeBlockCollectionEntry function is a great addition for centralizing this complex logic. The changes are thoroughly commented and include relevant test cases. I've identified a couple of areas for improvement in the new utility functions to enhance correctness and clarity.

pkgs/yaml_edit/lib/src/utils.dart

github-actions · 2025-11-07T12:26:08Z

Package publishing

Package	Version	Status	Publish tag (post-merge)
package:bazel_worker	1.1.4	already published at pub.dev
package:benchmark_harness	2.4.0	already published at pub.dev
package:boolean_selector	2.1.2	already published at pub.dev
package:browser_launcher	1.1.3	already published at pub.dev
package:cli_config	0.2.1-wip	WIP (no publish necessary)
package:cli_util	0.5.0-wip	WIP (no publish necessary)
package:clock	1.1.3-wip	WIP (no publish necessary)
package:code_builder	4.11.1-wip	WIP (no publish necessary)
package:coverage	1.15.0	already published at pub.dev
package:csslib	1.0.2	already published at pub.dev
package:extension_discovery	2.1.0	already published at pub.dev
package:file	7.0.2-wip	WIP (no publish necessary)
package:file_testing	3.1.0-wip	WIP (no publish necessary)
package:glob	2.1.3	already published at pub.dev
package:graphs	2.3.3-wip	WIP (no publish necessary)
package:html	0.15.7-wip	WIP (no publish necessary)
package:io	1.1.0-wip	WIP (no publish necessary)
package:json_rpc_2	4.1.0-wip	WIP (no publish necessary)
package:markdown	7.3.1-wip	WIP (no publish necessary)
package:mime	2.1.0-wip	WIP (no publish necessary)
package:oauth2	2.0.5	already published at pub.dev
package:package_config	2.3.0-wip	WIP (no publish necessary)
package:pool	1.5.2	already published at pub.dev
package:process	5.0.5	already published at pub.dev
package:pub_semver	2.2.0	already published at pub.dev
package:pubspec_parse	1.5.1-wip	WIP (no publish necessary)
package:source_map_stack_trace	2.1.3-wip	WIP (no publish necessary)
package:source_maps	0.10.14-wip	WIP (no publish necessary)
package:source_span	1.10.1	already published at pub.dev
package:sse	4.1.8	already published at pub.dev
package:stack_trace	1.12.2-wip	(error) pubspec version (1.12.2-wip) and changelog (1.12.2-dev) don't agree
package:stream_channel	2.1.4	already published at pub.dev
package:stream_transform	2.1.2-wip	WIP (no publish necessary)
package:string_scanner	1.4.1	already published at pub.dev
package:term_glyph	1.2.3-wip	WIP (no publish necessary)
package:test_reflective_loader	0.5.0	ready to publish	`test_reflective_loader-v0.5.0`
package:timing	1.0.2	already published at pub.dev
package:unified_analytics	8.0.9	ready to publish	`unified_analytics-v8.0.9`
package:watcher	1.1.5-wip	WIP (no publish necessary)
package:yaml	3.1.3	already published at pub.dev
package:yaml_edit	2.2.2	already published at pub.dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

github-actions · 2025-11-07T12:28:23Z

PR Health

License Headers ✔️

// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/bazel_worker/benchmark/benchmark.dart
pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart
pkgs/boolean_selector/example/example.dart
pkgs/clock/lib/clock.dart
pkgs/clock/lib/src/clock.dart
pkgs/clock/lib/src/default.dart
pkgs/clock/lib/src/stopwatch.dart
pkgs/clock/lib/src/utils.dart
pkgs/clock/test/clock_test.dart
pkgs/clock/test/default_test.dart
pkgs/clock/test/stopwatch_test.dart
pkgs/clock/test/utils.dart
pkgs/coverage/lib/src/coverage_options.dart
pkgs/html/example/main.dart
pkgs/html/lib/dom.dart
pkgs/html/lib/dom_parsing.dart
pkgs/html/lib/html_escape.dart
pkgs/html/lib/parser.dart
pkgs/html/lib/src/constants.dart
pkgs/html/lib/src/encoding_parser.dart
pkgs/html/lib/src/html_input_stream.dart
pkgs/html/lib/src/list_proxy.dart
pkgs/html/lib/src/query_selector.dart
pkgs/html/lib/src/token.dart
pkgs/html/lib/src/tokenizer.dart
pkgs/html/lib/src/treebuilder.dart
pkgs/html/lib/src/utils.dart
pkgs/html/test/dom_test.dart
pkgs/html/test/parser_feature_test.dart
pkgs/html/test/parser_test.dart
pkgs/html/test/query_selector_test.dart
pkgs/html/test/selectors/level1_baseline_test.dart
pkgs/html/test/selectors/level1_lib.dart
pkgs/html/test/selectors/selectors.dart
pkgs/html/test/support.dart
pkgs/html/test/tokenizer_test.dart
pkgs/html/test/trie_test.dart
pkgs/html/tool/generate_trie.dart
pkgs/pubspec_parse/test/git_uri_test.dart
pkgs/stack_trace/example/example.dart
pkgs/watcher/test/custom_watcher_factory_test.dart
pkgs/yaml_edit/example/example.dart

This check can be disabled by tagging the PR with skip-license-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package	Leaked API symbol	Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

Breaking changes ✔️

Package	Change	Current Version	New Version	Needed Version	Looking good?
yaml_edit	None	2.2.2	2.2.2	2.2.2	✔️

This check can be disabled by tagging the PR with skip-breaking-check.

Coverage

⚠️

File	Coverage
pkgs/yaml_edit/lib/src/equality.dart	💔 88 % ⬇️ 13 %
pkgs/yaml_edit/lib/src/list_mutations.dart	💔 99 % ⬇️ 0 %
pkgs/yaml_edit/lib/src/map_mutations.dart	💚 100 %
pkgs/yaml_edit/lib/src/utils.dart	💔 98 % ⬇️ 0 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

Changelog Entry ❗

Package	Changed Files
package:yaml_edit	pkgs/yaml_edit/lib/src/equality.dart pkgs/yaml_edit/lib/src/list_mutations.dart pkgs/yaml_edit/lib/src/map_mutations.dart pkgs/yaml_edit/lib/src/utils.dart

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

sigurdm · 2025-12-04T10:11:08Z

Does this preserve non-indented trailing comments?

- value
- >+
  folded keep chomp

- removed # Funky comment
# Not indented, is this removed?
- next

kekavc24 · 2025-12-04T11:01:44Z

Yes. # is on the same indent level as the list itself.

mosuem · 2025-12-12T14:44:58Z

@dart-lang/dart-pub-team I believe this needs a review :)

jonasfj

Sorry, it took long to do a review.

WOW, there is a lot of corner cases here.

This is really hard to follow. To be fair, it's also solving a LOT of cases!
I'm not saying there is much better way to do this.

It's just really complicated to keep even half the cases that this handles in your head at the same time 🤣

My comments are mostly just nits, nothing substantial.

I've reviewed the code, and tried to follow it (managed to understand some of it).
But unless I start playing with making examples and tweaking everything I don't think I can confidently claim I understand all the corner cases -- I'm simply not that smart 🤣

I also assume that you've spent a non-trivial amount of time figuring out all this! It's very impressive!

I think we should land it. Even if there is a few nits, of course it'd be great to fix them, but it's just nits.

@sigurdm, would you mind giving it a superficial read through, if you have no major concerns I suggest we ship it.

jonasfj · 2025-12-17T14:09:24Z

pkgs/yaml_edit/lib/src/utils.dart

+      end -= lineEnding == '\r\n' ? 2 : 1;
+    }
+
+    replacement = isBlockList ? '[]' : '{}';


nit: but I'd suggest returning early.

Suggested change

replacement = isBlockList ? '[]' : '{}';

return SourceEdit(start, end - start, isBlockList ? '[]' : '{}');

Similarly, in the if (!isLastEntry) { block, then you remove at-least one level of nesting.

Not sure that makes what happens easier to understand, but I can see that it's complex for a reason.

Makes sense. It was intentional. Returning once looked cleaner. Will fix this to reduce the nesting.

jonasfj · 2025-12-17T14:11:14Z

pkgs/yaml_edit/lib/src/utils.dart

+    final (:nearestLineEnding, :nextNodeColStart) = nextBlockNodeInfo();
+    final trueEndOffset = end - 1;
+
+    /// Make compact only if we are pointing to same line break from different


nit. technically these are not documentation comment, just comments so // would be more accurate than ///.

Not that this is important.

😁 force of habit. No problem.

jonasfj · 2025-12-17T14:16:28Z

pkgs/yaml_edit/test/utils_test.dart

+        expect(
+          indexOfCompactChar(
+            yaml,
+            mapKeys.elementAt(1).span.start.offset,


Suggested change

mapKeys.elementAt(1).span.start.offset,

mapKeys[1].span.start.offset,

Aren't you just taking the second element of a list? I guess maybe do .toList()

jonasfj · 2025-12-17T14:16:41Z

pkgs/yaml_edit/test/utils_test.dart

+          yaml,
+        ) as YamlMap)
+            .nodes
+            .keys


Suggested change

.keys

.keys

.toList()

Just to avoid elementAt(1) which doesn't make the code less cryptic 🤣

jonasfj · 2025-12-17T14:31:34Z

I'd note that more tests would always be appreciated.

Especially, the kinds of tests we have in:
https://github.com/dart-lang/tools/tree/main/pkgs/yaml_edit/test/testdata

Because even if we don't care to manually write the .golden files, and even if reviews of these are limited. The self-test logic we have that decodes the YAML and compares it with the expected result does offer a lot of value.

kekavc24 · 2025-12-18T06:44:40Z

I'd note that more tests would always be appreciated.

Especially, the kinds of tests we have in: https://github.com/dart-lang/tools/tree/main/pkgs/yaml_edit/test/testdata

Certainly! I remember. I just needed the go-ahead (review) to edit the existing failing tests that are affected by this change. Will work on this and include more edge cases.

jonasfj · 2025-12-18T07:23:46Z

I just needed the go-ahead (review) to edit the existing failing tests that are affected by this change. Will work on this.

Absolutely, if there are existing test cases affected by this change, we should update them.

Then we can review if those changes are problematic or not, my guess is that we should be willing to eat a few more comments and be willing to accept slightly less pretty output in exchange for fewer bugs!

sigurdm · 2025-12-18T08:59:31Z

pkgs/yaml_edit/test/remove_test.dart

+
+target-key: value # Comment
+                    # Comment
+next: value


Let's also have a test that non-indented comments are kept

key: >+ folded with keep chomping target-key: value # Comment # This comment should stay Comment viewnext: value

Thanks. This was flaky for maps. Fixed it in the latest changes.

sigurdm

I see no red flags in the code, but agree the code is highly nontrivial, and I'm not sure I follow all the details.

Yeah, we need to fix all existing tests, and that would also make it easier to see the impact of the change when reviewing.

* Refactor `_removeFromBlockList` and `_removeFromBlockMap` and remove dangling line breaks which may affect how block scalars are intepreted by the `package: yaml`. * Prefer obtaining an entry in one pass from `YamlMap`. * Add tests

kekavc24 · 2025-12-19T02:21:28Z

@jonasfj I saw #2285 and will definitely do that for the next few PR(s). Changes in this PR for YamlEditor.remove pass for all the files you set to be ignored (if you comment out the insert and update test paths).

Note

insert, update still use the old buggy code for block lists and maps which this PR did not fix.

Hope to use this as the baseline once merged.

kekavc24 requested a review from a team as a code owner November 7, 2025 11:58

github-actions bot added the package:yaml_edit label Nov 7, 2025

gemini-code-assist bot reviewed Nov 7, 2025

View reviewed changes

pkgs/yaml_edit/lib/src/utils.dart Outdated Show resolved Hide resolved

pkgs/yaml_edit/lib/src/utils.dart Outdated Show resolved Hide resolved

kekavc24 force-pushed the fix-line-ending-bug branch from 01a4b9d to bf49685 Compare November 7, 2025 12:25

kekavc24 force-pushed the fix-line-ending-bug branch from 111d8e6 to 3d394f6 Compare November 26, 2025 14:14

jonasfj reviewed Dec 17, 2025

View reviewed changes

sigurdm reviewed Dec 18, 2025

View reviewed changes

jonasfj mentioned this pull request Dec 18, 2025

Simple crash_tests for yaml_edit #2285

Merged

kekavc24 added 8 commits December 19, 2025 00:28

Use correct node span from the next node

0dc457a

Check first non-space match for compact-inline notation char

52b8c90

Preserve non-indented comments

15c014e

Refactor nested code and fix indent issue

04c6a21

Handle empty block sequences correctly

0a07e65

Update old tests to match changes

db122d5

Add tests

db86a0c

kekavc24 force-pushed the fix-line-ending-bug branch from 2127595 to db86a0c Compare December 19, 2025 01:30

kekavc24 requested review from jonasfj and sigurdm December 19, 2025 01:33

Default to map start offset for the first entry

f9beeed

	replacement = isBlockList ? '[]' : '{}';
	return SourceEdit(start, end - start, isBlockList ? '[]' : '{}');

	mapKeys.elementAt(1).span.start.offset,
	mapKeys[1].span.start.offset,

Prevent YamlEditor.remove from affecting existing block scalars #2240

Are you sure you want to change the base?

Prevent YamlEditor.remove from affecting existing block scalars #2240

Conversation

kekavc24 commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue(s)

Changes

Disclaimers

Rationale

Case 1: Top level list

Case 2: Top level maps

Case 3: Compact lists

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Package publishing

Uh oh!

github-actions bot commented Nov 7, 2025

PR Health

Uh oh!

sigurdm commented Dec 4, 2025

Uh oh!

kekavc24 commented Dec 4, 2025

Uh oh!

mosuem commented Dec 12, 2025

Uh oh!

jonasfj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasfj commented Dec 17, 2025

Uh oh!

kekavc24 commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasfj commented Dec 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigurdm left a comment

Choose a reason for hiding this comment

Uh oh!

kekavc24 commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Prevent `YamlEditor.remove` from affecting existing block scalars #2240

Prevent `YamlEditor.remove` from affecting existing block scalars #2240

kekavc24 commented Nov 7, 2025 •

edited

Loading

github-actions bot commented Nov 7, 2025 •

edited

Loading

kekavc24 commented Dec 18, 2025 •

edited

Loading