Skip to content

Pubspec parser clean-up and tweaks. #2215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Pubspec parser clean-up and tweaks. #2215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 67 additions & 64 deletions pkgs/pubspec_parse/lib/src/dependency.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// 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.

import 'package:collection/collection.dart';
import 'package:json_annotation/json_annotation.dart';
import 'package:pub_semver/pub_semver.dart';
import 'package:yaml/yaml.dart';
Expand Down Expand Up @@ -46,18 +45,24 @@ Dependency? _fromJson(Object? data, String name) {
}

if (data is Map) {
final matchedKeys = data.keys
.cast<String>()
.where((key) => key != 'version')
.toList();
String? matchedKey;
String? secondMatchedKey;
String? firstUnrecognizedKey;
for (final (key as String) in data.keys) {
if (key == 'version') continue;
if (matchedKey == null) {
matchedKey = key;
} else {
secondMatchedKey ??= key;
}
if (!_sourceKeys.contains(key)) {
firstUnrecognizedKey ??= key;
}
}

if (data.isEmpty || (matchedKeys.isEmpty && data.containsKey('version'))) {
if (matchedKey == null) {
return _$HostedDependencyFromJson(data);
} else {
final firstUnrecognizedKey = matchedKeys.firstWhereOrNull(
(k) => !_sourceKeys.contains(k),
);

return $checkedNew<Dependency>('Dependency', data, () {
if (firstUnrecognizedKey != null) {
throw UnrecognizedKeysException(
Expand All @@ -66,20 +71,18 @@ Dependency? _fromJson(Object? data, String name) {
_sourceKeys,
);
}
if (matchedKeys.length > 1) {
if (secondMatchedKey != null) {
throw CheckedFromJsonException(
data,
matchedKeys[1],
secondMatchedKey,
'Dependency',
'A dependency may only have one source.',
);
}

final key = matchedKeys.single;

return switch (key) {
'git' => GitDependency.fromData(data[key]),
'path' => PathDependency.fromData(data[key]),
return switch (matchedKey) {
'git' => GitDependency.fromData(data[matchedKey]),
'path' => PathDependency.fromData(data[matchedKey]),
'sdk' => _$SdkDependencyFromJson(data),
'hosted' => _$HostedDependencyFromJson(
data,
Expand Down Expand Up @@ -125,17 +128,8 @@ class GitDependency extends Dependency {

GitDependency(this.url, {this.ref, this.path});

factory GitDependency.fromData(Object? data) {
if (data is String) {
data = {'url': data};
}

if (data is Map) {
return _$GitDependencyFromJson(data);
}

throw ArgumentError.value(data, 'git', 'Must be a String or a Map.');
}
factory GitDependency.fromData(Object? data) =>
_$GitDependencyFromJson(_mapOrStringUri(data, 'git'));

@override
bool operator ==(Object other) =>
Expand All @@ -156,34 +150,37 @@ Uri? parseGitUriOrNull(String? value) =>

Uri parseGitUri(String value) => _tryParseScpUri(value) ?? Uri.parse(value);

/// Supports URIs like `[user@]host.xz:path/to/repo.git/`
/// Parses URIs like `[user@]host.xz:path/to/repo.git/`.
/// See https://git-scm.com/docs/git-clone#_git_urls_a_id_urls_a
Uri? _tryParseScpUri(String value) {
final colonIndex = value.indexOf(':');

if (colonIndex < 0) {
return null;
} else if (colonIndex == value.indexOf('://')) {
// If the first colon is part of a scheme, it's not an scp-like URI
return null;
}
final slashIndex = value.indexOf('/');

if (slashIndex >= 0 && slashIndex < colonIndex) {
// Per docs: This syntax is only recognized if there are no slashes before
// the first colon. This helps differentiate a local path that contains a
// colon. For example the local path foo:bar could be specified as an
// absolute path or ./foo:bar to avoid being misinterpreted as an ssh url.
return null;
}

final atIndex = value.indexOf('@');
if (colonIndex > atIndex) {
final user = atIndex >= 0 ? value.substring(0, atIndex) : null;
final host = value.substring(atIndex + 1, colonIndex);
final path = value.substring(colonIndex + 1);
return Uri(scheme: 'ssh', userInfo: user, host: host, path: path);
// Find first `:`. Remember `@` before it, reject if `/` before it.
const slashChar = 0x2F, colonChar = 0x3A, atChar = 0x40;
var atIndex = -1;
for (var i = 0; i < value.length; i++) {
final char = value.codeUnitAt(i);
if (char == slashChar) {
// Per docs: This syntax is only recognized if there are no slashes
// before the first colon. This helps differentiate a local path that
// contains a colon. For example the local path foo:bar could
// be specified as an absolute path or ./foo:bar to avoid being
// misinterpreted as an SSH URL
break;
} else if (char == atChar) {
atIndex = i;
} else if (char == colonChar) {
final colonIndex = i;

// Assume a `://` means it's a real URI scheme and authority,
// not an SCP-like URI.
if (value.startsWith('//', colonIndex + 1)) return null;

final user = atIndex >= 0 ? value.substring(0, atIndex) : null;
final host = value.substring(atIndex + 1, colonIndex);
final path = value.substring(colonIndex + 1);
return Uri(scheme: 'ssh', userInfo: user, host: host, path: path);
}
}
// No colon in value, or not before a slash.
return null;
}

Expand Down Expand Up @@ -257,17 +254,8 @@ class HostedDetails {

HostedDetails(this.declaredName, this.url);

factory HostedDetails.fromJson(Object data) {
if (data is String) {
data = {'url': data};
}

if (data is Map) {
return _$HostedDetailsFromJson(data);
}

throw ArgumentError.value(data, 'hosted', 'Must be a Map or String.');
}
factory HostedDetails.fromJson(Object data) =>
_$HostedDetailsFromJson(_mapOrStringUri(data, 'hosted'));

@override
bool operator ==(Object other) =>
Expand All @@ -279,3 +267,18 @@ class HostedDetails {

VersionConstraint _constraintFromString(String? input) =>
input == null ? VersionConstraint.any : VersionConstraint.parse(input);

/// 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;
}
Comment on lines +271 to +284
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.');
}

Copy link
Member Author

@lrhn lrhn Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, that's even better.

Copy link
Member

@natebosch natebosch Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW my personal preference is a switch.

Suggested change
/// 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.'),
};

Copy link
Member Author

@lrhn lrhn Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. That is even nicer.

118 changes: 57 additions & 61 deletions pkgs/pubspec_parse/lib/src/pubspec.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class Pubspec {
@Deprecated('See https://dart.dev/tools/pub/pubspec#authorauthors')
String? get author {
if (authors.length == 1) {
return authors.single;
return authors.first;
}
return null;
}
Expand Down Expand Up @@ -140,9 +140,9 @@ class Pubspec {
throw ArgumentError.value(name, 'name', '"name" cannot be empty.');
}

if (publishTo != null && publishTo != 'none') {
if (publishTo case final publishTo? when publishTo != 'none') {
try {
final targetUri = Uri.parse(publishTo!);
final targetUri = Uri.parse(publishTo);
if (!(targetUri.isScheme('http') || targetUri.isScheme('https'))) {
throw const FormatException('Must be an http or https URL.');
}
Expand All @@ -160,10 +160,10 @@ class Pubspec {
return _$PubspecFromJson(json);
} on CheckedFromJsonException catch (e) {
if (e.map == json && json.containsKey(e.key)) {
json = Map.from(json)..remove(e.key);
continue;
json = {...json}..remove(e.key);
} else {
rethrow;
}
rethrow;
}
}
}
Expand All @@ -183,67 +183,63 @@ class Pubspec {
);

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,
];
}
Comment on lines 185 to 193
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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();
  }

Copy link
Member Author

@lrhn lrhn Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}];.

Copy link
Member

@natebosch natebosch Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Version? _versionFromString(String? input) =>
input == null ? null : Version.parse(input);

Map<String, VersionConstraint?> _environmentMap(Map? source) =>
source?.map((k, value) {
final key = k as String;
if (key == 'dart') {
// github.com/dart-lang/pub/blob/d84173eeb03c3/lib/src/pubspec.dart#L342
// 'dart' is not allowed as a key!
throw CheckedFromJsonException(
source,
'dart',
'VersionConstraint',
'Use "sdk" to for Dart SDK constraints.',
badKey: true,
);
}

VersionConstraint? constraint;
if (value == null) {
constraint = null;
} else if (value is String) {
try {
constraint = VersionConstraint.parse(value);
} on FormatException catch (e) {
throw CheckedFromJsonException(source, key, 'Pubspec', e.message);
}
Map<String, VersionConstraint?> _environmentMap(Map? source) {
if (source == null) return {};
return source.map((k, value) {
final key = k as String;
if (key == 'dart') {
// github.com/dart-lang/pub/blob/d84173eeb03c3/lib/src/pubspec.dart#L342
// 'dart' is not allowed as a key!
throw CheckedFromJsonException(
source,
'dart',
'VersionConstraint',
'Use "sdk" to for Dart SDK constraints.',
badKey: true,
);
}

return MapEntry(key, constraint);
} else {
throw CheckedFromJsonException(
source,
key,
'VersionConstraint',
'`$value` is not a String.',
);
VersionConstraint? constraint;
if (value is String) {
try {
constraint = VersionConstraint.parse(value);
} on FormatException catch (e) {
throw CheckedFromJsonException(source, key, 'Pubspec', e.message);
}
} else if (value != null) {
throw CheckedFromJsonException(
source,
key,
'VersionConstraint',
'`$value` is not a String.',
);
}
return MapEntry(key, constraint);
});
}

return MapEntry(key, constraint);
}) ??
{};

Map<String, String?> _executablesMap(Map? source) =>
source?.map((k, value) {
final key = k as String;
if (value == null) {
return MapEntry(key, null);
} else if (value is String) {
return MapEntry(key, value);
} else {
throw CheckedFromJsonException(
source,
key,
'String',
'`$value` is not a String.',
);
}
}) ??
{};
Map<String, String?> _executablesMap(Map? source) => {
if (source != null)
for (final MapEntry(:key as String, :value) in source.entries)
key: (value is String?)
? value
: (throw CheckedFromJsonException(
source,
key,
'String',
'`$value` is not a String.',
)),
};
Loading
Loading