-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Replace PackageGraphRoot.PackageDependency with PackageDependencyDescription #2997
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.
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
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this cleanup! I had one question about what looks as if it might be a semantic difference, but I might of course be reading the code wrong. Apart from that this looks good.
return PackageReference( | ||
identity: PackageReference.computeIdentity(packageURL: effectiveURL), | ||
identity: explicitName?.lowercased() ?? PackageReference.computeIdentity(packageURL: effectiveURL), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a change in semantics. Are there any risks here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. If you remove the left-hand side of that ??
expression, tests will fail, with reports of mismatched dependency names. So although this looks like a semantic change, it's merely a downstream effect of consolidating PackageGraphRoot.PackageDependency
into PackageDependencyDescription
.
Later on, we will be changing the semantics so that we only use the package URL (normalized for scheme, case, etc.) for identity. Any explicit names would then become convenient labels rather than something load-bearing — more like a variable name within a block than a memory address. But to be clear: that's not what this change is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see; thanks for the explanation. If this doesn't have any actual effect, it would be great if there were a comment explaining what's going on. If this will go away when the notion of package identity changes, then this seems OK for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Added with 0ccdeaf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well, no other comments than the ones Anders already made.
FYI — Currently getting a 502 Proxy Error for the details on the failed smoke test.
|
This comment has been minimized.
This comment has been minimized.
@swift-ci please smoke test |
Getting 502 proxy errors again (see #2997 (comment)) |
^ Seems like an unrelated one-off. Trying again. |
@swift-ci please smoke test |
@abertelrud Any more requested changes? Or is this ready to be squashed and merged? |
This looks good to me at this point. Thanks! Are you able to merge it or should I do it? |
@abertelrud It looks like I'm able to do it, so I'll go ahead and merge now. |
…ription (swiftlang#2997) * Replace PackageGraphRoot.PackageDependency with PackageDependencyDescription * Add documentation comment for nothing type property * Update CMakeLists.txt for PackageModel module * Replace reference to FilteredDependencyDescription * Add comment about how package identity is determined
As discussed with @neonichu and @aciidb0mb3r (see this comment), this PR replaces all use of
PackageGraphRoot.PackageDependency
withPackageDependencyDescription
. This gets us one step closer to having a single, canonical type capable of representing package dependencies to the extent required to implement the package registry (#2967).This PR also does away with
FilteredDependencyDescription
, which now has an equivalent representation by way of theproductFilter
property added toPackageDependencyDescription
.