Conversation
Closes #2351 Closes #2350 I cannot find evidence of use case for writing a string literal that includes interpolation. Drop the generality and update the doc to describe a behavior where the content of the resulting string always matches the content of the argument. Use an implementation from `package:source_helper` that attempts to choose an idiomatic delimiter and falls back to single quotes with every character escaped. Bump major version since this is technically breaking. Most team usages should be able to use an expanded constraint.
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with
Coverage
|
| File | Coverage |
|---|---|
| pkgs/code_builder/lib/src/specs/expression/literal.dart | 💔 Not covered |
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.
Unused Dependencies ⚠️
| Package | Status |
|---|---|
| code_builder | ❗ Show IssuesThese packages may be unused, or you may be using assets from these packages: |
For details on how to fix these, see dependency_validator.
This check can be disabled by tagging the PR with skip-unused-dependencies-check.
Breaking changes ✔️
| Package | Change | Current Version | New Version | Needed Version | Looking good? |
|---|---|---|---|---|---|
| code_builder | Breaking | 4.11.1 | 5.0.0-wip | 5.0.0-wip | ✔️ |
This check can be disabled by tagging the PR with skip-breaking-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.
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/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/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/watcher/test/custom_watcher_factory_test.dart |
This check can be disabled by tagging the PR with skip-license-check.
|
Ugh, I forgot that |
![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 implements a breaking change to literalString, removing the raw parameter in favor of an automated escaping mechanism that handles single and double quotes, dollar signs, and control characters. The package version is updated to 5.0.0-wip. Review feedback highlights logic errors in the newly added test cases regarding the expected output for strings with dollar signs. Suggestions were also provided to avoid variable shadowing in the escaping logic and to optimize performance by defining the escape map regular expression as a constant.
| }); | ||
|
|
||
| test('should use raw single quotes for dollar signs if possible', () { | ||
| expect(literalString(r'$foo'), equalsDart('r\'\$foo\'')); |
There was a problem hiding this comment.
This test appears to be incorrect. The input string to literalString is '$foo'. The generated code should produce this exact string. The implementation correctly generates r'$foo', which evaluates to '$foo'. However, the test expects r'\$foo', which evaluates to '\$foo', a different string containing a literal backslash.
| expect(literalString(r'$foo'), equalsDart('r\'\$foo\'')); | |
| expect(literalString(r'$foo'), equalsDart(r"'\$foo'")); |
There was a problem hiding this comment.
The \$ in the test source is escaping at that level, we don't have raw string here, we have non-raw string which contains a raw string. This is already correct, but it's easier to read with equalsDart(r"r'$foo'").
|
|
||
| test('should use raw double quotes for dollar signs and single quotes ' | ||
| 'if possible', () { | ||
| expect(literalString(r"don't $foo"), equalsDart('r"don\'t \$foo"')); |
There was a problem hiding this comment.
This test seems to be incorrect. The input string is don't $foo. The generated code should produce this exact string. The implementation correctly generates r"don't $foo", which evaluates to don't $foo. However, the test expects r"don't \$foo", which evaluates to don't \$foo, a different string containing a literal backslash.
expect(literalString(r"don't $foo"), equalsDart(r'r"don\'t $foo"'));| var canBeRaw = true; | ||
|
|
||
| value = value.replaceAllMapped(_escapeRegExp, (match) { | ||
| final value = match[0]!; |
There was a problem hiding this comment.
According to the Effective Dart style guide, which this repository follows, you should avoid shadowing names. The variable value declared here inside the replaceAllMapped callback shadows the value parameter of the _escapeString function. This can make the code harder to read and understand.
Consider renaming this variable to something like char or matchString and updating its usages within this block.
| final value = match[0]!; | |
| final char = match[0]!; |
References
- This repository's style guide requires following Effective Dart. Effective Dart's style guide recommends avoiding shadowing names to improve readability and prevent bugs. (link)
| /// single-quote, double-quote, and `$` | ||
| final _escapeRegExp = RegExp('[\$\'"\\x00-\\x07\\x0E-\\x1F$_escapeMapRegexp]'); | ||
|
|
||
| final _escapeMapRegexp = _escapeMap.keys.map(_hexLiteral).join(); |
There was a problem hiding this comment.
The value of _escapeMapRegexp is constant and can be pre-calculated to avoid runtime computation. This improves performance and makes the code more readable.
Please define this as a const string and move its definition before _escapeRegExp.
| final _escapeMapRegexp = _escapeMap.keys.map(_hexLiteral).join(); | |
| const _escapeMapRegexp = r'\x08\x09\x0A\x0B\x0C\x0D\x7F\x5C'; |
Avoid a dependency cycle with `package:build_runner` during the major version bump. We won't be able to regenerate `built_value` classes until we update build runner for compatibility
|
@davidmorgan - this change will impact Since mockito is google3 SoT I plan to:
Do you have any concerns? |
1 participant
Closes #2351
Closes #2350
I cannot find evidence of use case for writing a string literal that
includes interpolation. Drop the generality and update the doc to
describe a behavior where the content of the resulting string always
matches the content of the argument. Use an implementation from
package:source_helperthat attempts to choose an idiomatic delimiterand falls back to single quotes with every character escaped.
Bump major version since this is technically breaking. Most team usages
should be able to use an expanded constraint.