Skip to content

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Mar 18, 2024

Part of #4127

@sigurdm sigurdm requested review from jonasfj and szakarias March 18, 2024 13:27
withPubspecOverrides: true,
);
for (final package in root.transitiveWorkspace) {
pubspecsMet.entries.first;
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops. Removed.

Copy link
Member

Choose a reason for hiding this comment

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

is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it is :)

);
for (final package in root.transitiveWorkspace) {
pubspecsMet.entries.first;
if (identical(pubspecsMet.entries.first.value, package.pubspec)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to find the Package object that has the first Pubspec that we loaded. Such that we can refer it.

Copy link
Member

Choose a reason for hiding this comment

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

Are you relying on the iteration order for .entries -- that's seems a little surprising.

Maybe, make like a firstPubspecMet variable that is nullable?

Is this because the root loads the packages? Ah.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can rely on the insertion order of a map.

@jonasfj
Copy link
Member

jonasfj commented Mar 18, 2024

I don't really have any reservations here. It's probably, but perhaps a few documentation comments would help us review it :D

);
for (final package in root.transitiveWorkspace) {
pubspecsMet.entries.first;
if (identical(pubspecsMet.entries.first.value, package.pubspec)) {
Copy link
Member

Choose a reason for hiding this comment

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

Are you relying on the iteration order for .entries -- that's seems a little surprising.

Maybe, make like a firstPubspecMet variable that is nullable?

Is this because the root loads the packages? Ah.

String dir,
SourceRegistry sources, {
bool withPubspecOverrides = false,
Map<String, Pubspec> alreadyLoadedPubspecs = const {},
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this, how about passing a loadPubspec() function?

Then you won't need to document the structure of the map, and the fact that things are cached is something you can hide altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sigurdm sigurdm merged commit 2179b76 into dart-lang:master Mar 22, 2024
atsansone pushed a commit to dart-lang/site-www that referenced this pull request Mar 22, 2024
Part of dart-lang/pub#4127
This link is referred in dart-lang/pub#4186

Later it should be updated to point at the actual documentation. For now
it is pointing at the design doc.
atsansone pushed a commit to atsansone/site-www that referenced this pull request Mar 22, 2024
Part of dart-lang/pub#4127
This link is referred in dart-lang/pub#4186

Later it should be updated to point at the actual documentation. For now
it is pointing at the design doc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants