Repository navigation
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 adds support for exposing the version constraint key in GitDependency and PathDependency instead of silently discarding it. It updates constructors, serialization, equality checks, and adds corresponding tests. Feedback was provided regarding PathDependency.fromData where casting version as String? could lead to a runtime TypeError if a VersionConstraint object is passed directly; a type check is suggested to make the factory constructor more robust.
2184543 to
6caacfe
Compare
hkarmoush
changed the title
|
Addressed the review comment in 6caacfe (and rebased):
The |
|
Could you make three quick updates so we can land this?
|
A `version` key alongside `path` is silently dropped today, even though pub's solver still validates it against the resolved package's version. Parse and expose it instead of discarding it, matching the `version` field already exposed on GitDependency. Generalizes `_optionalConstraintFromData` (previously GitDependency-only) to take a className for its error messages, so both dependency types get correctly attributed parse errors.
6caacfe to
871f5e9
Compare
|
Thanks, all three done. Dropped the Co-Authored-By trailer from the commit message so the CLA check should pass now, rebased on main (kept both changelog entries), and added a PathDependency with a version constraint to the toJson test, including the round trip. |
|
please fix the changelog churn! (just landed those, sorry) |
…-path-version-constraint-1809 # Conflicts: # pkgs/pubspec_parse/CHANGELOG.md
tools (https://github.com/dart-lang/tools/compare/f6c7a2d..4d7715a): 4d7715ac Mon Oct 19 16:09:30 2026 -0700 Kevin Moore Replace `@dart-lang/dart-bat` with `@dart-lang/dart-ecosystem-team` in `CODEOWNERS` (dart-lang/tools#2697) a22c558c Tue Oct 20 06:56:10 2026 +0900 YeungKC [markdown] Check the first character before inline syntax regexes (dart-lang/tools#2695) c57bde49 Mon Oct 19 14:40:39 2026 -0700 dart-ecosystem-bot[bot] [boring] Update BoringSSL to 64188f17 (dart-lang/tools#2692) 7a4ea6f8 Tue Oct 20 00:09:47 2026 +0300 Hasan Armoush [pubspec_parse] Expose version constraint on PathDependency (dart-lang/tools#2581) 85e03753 Mon Oct 19 22:41:32 2026 +0200 Lukas Klingsbo [pubspec_parse] Add hooks field to Pubspec (dart-lang/tools#2568) 13139ec6 Mon Oct 19 22:28:47 2026 +0200 Lukas Klingsbo [pubspec_parse] Add false_secrets field to Pubspec (dart-lang/tools#2567) 462d92bf Mon Oct 19 21:56:25 2026 +0200 Lukas Klingsbo [pubspec_parse] Add platforms field to Pubspec (dart-lang/tools#2566) TEST=ci Change-Id: I106b05a08a7395f72e01a86a952abd41f4630831 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/566840 Commit-Queue: Kevin Moore <[email protected]> Reviewed-by: Nate Bosch <[email protected]>
|
Thanks for the contribution! Published in |
A
versionkey alongsidepathis silently dropped today, even though pub's solver still validates it against the resolved package's version. Parse and expose it instead of discarding it, matching theversionfield already exposed onGitDependency.(This originally also touched
GitDependency, but that's since landed separately via the tag-pattern work, so this PR is now scoped toPathDependencyonly.)Fixes #1809
Test plan
dart test— all 122 tests passdart analyze— no issuesdart format— cleanpathdep with aversionkey, default (null) when absent, and invalid/non-Stringversionvalues