Pubspec parser clean-up and tweaks. #2215
Conversation
Avoid iterating the same thing more than once. Use newer language features wher appropriate. Put duplicate code into helper functions.
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
![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 does a great job of cleaning up the pubspec parser. The changes to avoid multiple iterations, use newer language features like patterns, and extract repeated code into helper functions are all excellent improvements for code quality and performance. I've found a couple of issues. One is a small documentation error and an opportunity for simplification in a new helper function. The other is a potential regression in the _normalizeAuthors function where it no longer deduplicates all authors. My review includes suggestions to address these points. Overall, this is a solid refactoring effort.
| static List<String> _normalizeAuthors(String? author, List<String>? authors) { | ||
| final value = <String>{if (author != null) author, ...?authors}; | ||
| return value.toList(); | ||
| if (author == null) return authors ?? const []; | ||
| return [ | ||
| author, | ||
| if (authors != null) | ||
| for (var otherAuthor in authors) | ||
| if (author != otherAuthor) otherAuthor, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
The new implementation of _normalizeAuthors no longer removes duplicate entries within the authors list itself, and also doesn't remove duplicates if author is null. The previous implementation used a Set, which correctly handled all duplicates. The constructor comment for Pubspec also states that duplicates are eliminated. This change is an unintended regression.
For example, if authors is ['a', 'a'] and author is null, the old implementation would return ['a'], while the new one returns ['a', 'a'].
To preserve the old behavior of removing all duplicates, it's best to use a Set as before.
static List<String> _normalizeAuthors(String? author, List<String>? authors) {
final value = <String>{if (author != null) author, ...?authors};
return value.toList();
}There was a problem hiding this comment.
Correctly spotted. Is this something we care about?
(There is no test which fails, so clearly not.)
Are we normalizing for our own sake, or for the user's, if they pass in a list with duplicate authors.
And Gemini, you can write that entire body as => [...{?author, ...?authors}];.
There was a problem hiding this comment.
I doubt deduplication within authors is an intended feature and I don't think it's likely to come up in practice. I'd be happy with either using this version, or restoring the deduplicating behavior and adding a test. I think you're already handling the important name to dedupe.
| /// The `value` if it is a `Map`, or `{'uri': value}` if it is a `String`. | ||
| /// | ||
| /// Must be one or the other. | ||
| /// The [name] is used as parameter name in the thrown error. | ||
| Map _mapOrStringUri(Object? value, String name) { | ||
| if (value is! Map) { | ||
| if (value is String) { | ||
| value = {'url': value}; | ||
| } else { | ||
| throw ArgumentError.value(value, name, 'Must be a String or a Map.'); | ||
| } | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
This function can be simplified to avoid reassigning the value parameter, which improves readability. Also, the documentation has a small error: it mentions {'uri': value} but the implementation uses {'url': value}. The suggested code fixes both issues.
/// The `value` if it is a `Map`, or `{'url': value}` if it is a `String`.
///
/// Must be one or the other.
/// The [name] is used as parameter name in the thrown error.
Map _mapOrStringUri(Object? value, String name) {
if (value is Map) {
return value;
}
if (value is String) {
return {'url': value};
}
throw ArgumentError.value(value, name, 'Must be a String or a Map.');
}There was a problem hiding this comment.
FWIW my personal preference is a switch.
| /// The `value` if it is a `Map`, or `{'uri': value}` if it is a `String`. | |
| /// | |
| /// Must be one or the other. | |
| /// The [name] is used as parameter name in the thrown error. | |
| Map _mapOrStringUri(Object? value, String name) { | |
| if (value is! Map) { | |
| if (value is String) { | |
| value = {'url': value}; | |
| } else { | |
| throw ArgumentError.value(value, name, 'Must be a String or a Map.'); | |
| } | |
| } | |
| return value; | |
| } | |
| /// The `value` if it is a `Map`, or `{'url': value}` if it is a `String`. | |
| /// | |
| /// Must be one or the other. | |
| /// The [name] is used as parameter name in the thrown error. | |
| Map _mapOrStringUri(Object? value, String name) => switch (value) { | |
| Map() => value, | |
| String() => {'url': value}, | |
| _ => throw ArgumentError.value(value, name, 'Must be a String or a Map.'), | |
| }; |
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with Changelog 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/pubspec_parse/lib/src/dependency.dart | 💔 71 % ⬇️ 2 % |
| pkgs/pubspec_parse/lib/src/pubspec.dart | 💚 100 % ⬆️ 2 % |
| pkgs/pubspec_parse/lib/src/screenshot.dart | 💚 100 % |
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.
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) 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.
| /// The `value` if it is a `Map`, or `{'uri': value}` if it is a `String`. | ||
| /// | ||
| /// Must be one or the other. | ||
| /// The [name] is used as parameter name in the thrown error. | ||
| Map _mapOrStringUri(Object? value, String name) { | ||
| if (value is! Map) { | ||
| if (value is String) { | ||
| value = {'url': value}; | ||
| } else { | ||
| throw ArgumentError.value(value, name, 'Must be a String or a Map.'); | ||
| } | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
FWIW my personal preference is a switch.
| /// The `value` if it is a `Map`, or `{'uri': value}` if it is a `String`. | |
| /// | |
| /// Must be one or the other. | |
| /// The [name] is used as parameter name in the thrown error. | |
| Map _mapOrStringUri(Object? value, String name) { | |
| if (value is! Map) { | |
| if (value is String) { | |
| value = {'url': value}; | |
| } else { | |
| throw ArgumentError.value(value, name, 'Must be a String or a Map.'); | |
| } | |
| } | |
| return value; | |
| } | |
| /// The `value` if it is a `Map`, or `{'url': value}` if it is a `String`. | |
| /// | |
| /// Must be one or the other. | |
| /// The [name] is used as parameter name in the thrown error. | |
| Map _mapOrStringUri(Object? value, String name) => switch (value) { | |
| Map() => value, | |
| String() => {'url': value}, | |
| _ => throw ArgumentError.value(value, name, 'Must be a String or a Map.'), | |
| }; |
| static List<String> _normalizeAuthors(String? author, List<String>? authors) { | ||
| final value = <String>{if (author != null) author, ...?authors}; | ||
| return value.toList(); | ||
| if (author == null) return authors ?? const []; | ||
| return [ | ||
| author, | ||
| if (authors != null) | ||
| for (var otherAuthor in authors) | ||
| if (author != otherAuthor) otherAuthor, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
I doubt deduplication within authors is an intended feature and I don't think it's likely to come up in practice. I'd be happy with either using this version, or restoring the deduplicating behavior and adding a test. I think you're already handling the important name to dedupe.
| String _readString(Map input, String entryName) { | ||
| final value = input[entryName]; | ||
| if (value is String) return value; | ||
| if (value == null) { | ||
| throw CheckedFromJsonException( | ||
| input, | ||
| entryName, | ||
| 'Screenshot', | ||
| 'Missing required key `$entryName`', | ||
| ); | ||
| } | ||
|
|
||
| for (final e in input) { | ||
| if (e is! Map) continue; | ||
|
|
||
| final description = e['description']; | ||
| if (description == null) { | ||
| throw CheckedFromJsonException( | ||
| e, | ||
| 'description', | ||
| 'Screenshot', | ||
| 'Missing required key `description`', | ||
| ); | ||
| } | ||
|
|
||
| if (description is! String) { | ||
| throw CheckedFromJsonException( | ||
| e, | ||
| 'description', | ||
| 'Screenshot', | ||
| '`$description` is not a String', | ||
| ); | ||
| } | ||
|
|
||
| final path = e['path']; | ||
| if (path == null) { | ||
| throw CheckedFromJsonException( | ||
| e, | ||
| 'path', | ||
| 'Screenshot', | ||
| 'Missing required key `path`', | ||
| ); | ||
| } | ||
|
|
||
| if (path is! String) { | ||
| throw CheckedFromJsonException( | ||
| e, | ||
| 'path', | ||
| 'Screenshot', | ||
| '`$path` is not a String', | ||
| ); | ||
| } | ||
|
|
||
| res.add(Screenshot(description, path)); | ||
| } | ||
| return res; | ||
| throw CheckedFromJsonException( | ||
| input, | ||
| entryName, | ||
| 'Screenshot', | ||
| '`$value` is not a String', | ||
| ); |
There was a problem hiding this comment.
[optional] I like a switch here too.
| String _readString(Map input, String entryName) { | |
| final value = input[entryName]; | |
| if (value is String) return value; | |
| if (value == null) { | |
| throw CheckedFromJsonException( | |
| input, | |
| entryName, | |
| 'Screenshot', | |
| 'Missing required key `$entryName`', | |
| ); | |
| } | |
| for (final e in input) { | |
| if (e is! Map) continue; | |
| final description = e['description']; | |
| if (description == null) { | |
| throw CheckedFromJsonException( | |
| e, | |
| 'description', | |
| 'Screenshot', | |
| 'Missing required key `description`', | |
| ); | |
| } | |
| if (description is! String) { | |
| throw CheckedFromJsonException( | |
| e, | |
| 'description', | |
| 'Screenshot', | |
| '`$description` is not a String', | |
| ); | |
| } | |
| final path = e['path']; | |
| if (path == null) { | |
| throw CheckedFromJsonException( | |
| e, | |
| 'path', | |
| 'Screenshot', | |
| 'Missing required key `path`', | |
| ); | |
| } | |
| if (path is! String) { | |
| throw CheckedFromJsonException( | |
| e, | |
| 'path', | |
| 'Screenshot', | |
| '`$path` is not a String', | |
| ); | |
| } | |
| res.add(Screenshot(description, path)); | |
| } | |
| return res; | |
| throw CheckedFromJsonException( | |
| input, | |
| entryName, | |
| 'Screenshot', | |
| '`$value` is not a String', | |
| ); | |
| String _readString(Map input, String entryName) => switch (input[entryName]) { | |
| final String value => value, | |
| null => throw CheckedFromJsonException( | |
| input, | |
| entryName, | |
| 'Screenshot', | |
| 'Missing required key `$entryName`', | |
| ), | |
| final value => throw CheckedFromJsonException( | |
| input, | |
| entryName, | |
| 'Screenshot', | |
| '`$value` is not a String', | |
| ), | |
| }; |
2 participants
Avoid iterating the same thing more than once (lists, strings using
indexOf).Use newer language features where appropriate.
Put repeated code into helper functions.