[yaml] Better recovering for list entry without - prefix - #2427
[yaml] Better recovering for list entry without - prefix#2427FMorschel wants to merge 4 commits into
- prefix#2427Conversation
![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 improves error recovery and messaging when parsing YAML block sequences with list elements missing the '- ' prefix. It introduces tracking of token types at each indentation level in the scanner (_indentTypes) to detect if a missing colon error occurs within a block sequence, appending a helpful hint to the error message. The parser is also updated to handle the recovered key token as a sequence entry, and a test case has been added to verify this behavior. There are no review comments, so no further feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Can you take a look or assign someone, please, @nex3? Thanks a lot! |
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthUnused Dependencies ✔️
For details on how to fix these, see dependency_validator. This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with
API leaks
|
| Package | Leaked API symbol | Leaking sources |
|---|---|---|
| yaml | ErrorListener | yaml/yaml.dart::loadYaml::errorListener yaml/yaml.dart::loadYamlNode::errorListener yaml/yaml.dart::loadYamlDocument::errorListener |
| yaml | ScalarEvent | yaml_node.dart::YamlScalar::internal::scalar |
| yaml | EventType | event.dart::Event::type event.dart::EventType::streamStart event.dart::EventType::streamEnd event.dart::EventType::documentStart event.dart::EventType::documentEnd event.dart::EventType::alias event.dart::EventType::scalar event.dart::EventType::sequenceStart event.dart::EventType::sequenceEnd event.dart::EventType::mappingStart event.dart::EventType::mappingEnd event.dart::EventType::values event.dart::Event::new::type event.dart::ScalarEvent::type |
This check can be disabled by tagging the PR with skip-leaking-check.
Changelog Entry ✔️
| Package | Changed Files |
|---|
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.
Coverage ✔️
| File | Coverage |
|---|---|
| pkgs/yaml/lib/src/parser.dart | 💚 94 % ⬆️ 0 % |
| pkgs/yaml/lib/src/scanner.dart | 💚 95 % ⬆️ 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.
License Headers ✔️
// Copyright (c) 2026, 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/coverage/lib/src/coverage_options.dart |
| pkgs/html/example/main.dart |
| pkgs/pubspec_parse/test/git_uri_test.dart |
| pkgs/watcher/test/custom_watcher_factory_test.dart |
This check can be disabled by tagging the PR with skip-license-check.
sigurdm
left a comment
sigurdm
left a comment
There was a problem hiding this comment.
Nice - just a small suggestion.
|
Thanks, @sigurdm! Done. |
FMorschel
changed the title
- prefix- prefix
|
I have a question. How does the rolling of this package into the Dart SDK work? Because I have a CL open (https://dart-review.googlesource.com/c/sdk/+/511260/1..2), which mentions the issue this fixes. How does this get rolled into the SDK if that test will start to fail if we remove the annotation before this lands, and will fail if this lands with the annotation there? |
I don't know how up-to-date my knowledge is, but my understanding is: Versions of external packages in the SDK are controlled by the DEPS file. For packages from this repo (tools), they are all controlled together by https://github.com/dart-lang/sdk/blob/c9b0df8469a2cb58298dde065044df8eceaaae5f/DEPS#L152 There's a script in the SDK tools/rev_sdk_deps.dart that automates bumping those versions and producing a commit message that includes a summary of the changes it's bringing in, and here's an example commit: So after this PR lands, that script could be used to produce a change, and the annotation removed in the same CL. When I've done this in the past, to avoid the CL being a noisy mix of dep revs and a functional change, I've selectively only included the one package update and made the commit message clear. |
| _scanner.scan(); | ||
| _states.add(_State.BLOCK_SEQUENCE_ENTRY); | ||
| return _parseNode(block: true); | ||
| } |
There was a problem hiding this comment.
This is entirely orthogonal to the changes in scanner.dart, except that now the scanner will most likely report the thing about how it expected a '-'.
And because if this, we are not okay using the token it insert to do some recovery? Instead of throwing YamlException?
There was a problem hiding this comment.
We can't be sure that is what the user meant to do. I read the documentation for yaml just to work on this change, so I'm not an expert, but from what I could gather, the user could still be trying to write a key one indentation before the list but got the indentation wrong.
This part merely resolves this by considering it a list entry and stops us from throwing the YamlException below if we are recovering. If we are not recovering, the exception in scanner.dart will flow all the way up.
See:
There was a problem hiding this comment.
@jonasfj, is the above explanation enough for you to merge this, or do you need anything else from me?
jonasfj
left a comment
jonasfj
left a comment
There was a problem hiding this comment.
I'm not 100% sure how all of this works 🙈
But at the top of my head, this looks reasonably safe, I won't block it.
|
@sigurdm any reason we don't merge this when you're back? |
4 participants
This is a suggestion to fix:
-prefix #2426Where we recover from a list item without the
-prefix and give it a more coherent error message.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.