Prevent YamlEditor.remove from affecting existing block scalars
#2240
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
Outdated
|
|
||
| // We scanned till the end of the string and found nothing. | ||
| if (currentOffset == -1) { | ||
| lineEndingIndex = currentOffset; |
There was a problem hiding this comment.
There seems to be a bug when indexOf returns -1. In that case, currentOffset becomes -1, and lineEndingIndex is also set to -1. This causes the caller, removeBlockCollectionEntry, to calculate an incorrect end offset for the source edit, potentially leading to incorrect document modifications.
When currentOffset is -1, it signifies that the rest of the string contains only whitespace. To ensure this trailing whitespace is included in the removal, the end of the removal range should extend to the end of the string. Setting lineEndingIndex to lastOffset will allow the caller to correctly calculate the end of the removal range as yaml.length.
| lineEndingIndex = currentOffset; | |
| lineEndingIndex = lastOffset; |
There was a problem hiding this comment.
Indeed. Missed that while renaming stuff.
pkgs/yaml_edit/lib/src/utils.dart
Outdated
| default: | ||
| break scanner; | ||
| } | ||
| } while (start >= 0); |
There was a problem hiding this comment.
The loop condition while (start >= 0) is a bit confusing as the start variable is not modified within the loop. The loop correctly terminates due to break or return statements, but the condition itself is misleading. Using while (true) would more clearly communicate the intent that this is a loop that is exited from within its body, improving code readability and maintainability.
| } while (start >= 0); | |
| } while (true); |
* 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
force-pushed
the
fix-line-ending-bug
branch
from
01a4b9d to
bf49685
Compare
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with 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.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with
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.
1 participant
Issue(s)
Changes
YamlEditor.removeon a block map/list now removes the entire node completely and leaves no dangling comments or line breaks._removeFromBlockMapand_removeFromBlockListhave 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
YamlEditoritself doesn't support insertingScalarStyle.FOLDEDandScalarStyle.LITERALstrings 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
Case 2: Top level maps
Case 3: Compact lists
Same logic applies to block maps.
Contribution guidelines:
dart format.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.