improve correctness by using identity more broadly throughout the code #3152
Conversation
tomerd
requested review from
abertelrud,
aciidgh,
friedbunny and
neonichu
as code owners
tomerd
changed the title
|
@swift-ci please smoke test |
motivation: improve correctlness by using identity more broadly throughout the code changes: * remove name from package-reference, replace with alternate-identity for secondary (manifest driven) identity * use identity to lookup managed-dependencies and pins instead of urls * refactor pins map serialization * adjust tests
* rename PackageReference::path to PackageReference::location to make its true meaning clearer * make PackageReference ctro take kind explicitly instead of defaulting to remote to make code more explicit * rename PackageContainer::identifer to PackageContainer::pacakge to reflect the correct type * adjust call-sites and other related code
tomerd
force-pushed
the
refactor/identity-1
branch
from
72eb799 to
689e70c
Compare
|
@swift-ci please smoke test |
|
cc @mattt |
There was a problem hiding this comment.
This is fantastic. Excellent work, @tomerd! This is something I tried (and failed) to implement a handful of times in the run-up to #3057. (My approach was to convert PackageReference to an enumeration of root / local / remote, but that was too cumbersome in practice; your approach to ease in with static constructors is smart)
For your consideration, I have some follow-up changes here: https://github.com/mattt/swift-package-manager/pull/1/files
My goal here was to eliminate some of the redundancy of initializing PackageIdentity values when creating package references. Not only for the net reduction in code, but to reenforce the relationship of package identity as being derived from the package reference. With this change, a PackageIdentity is only created directly if there isn't already a package reference or if we need to override something (such as for tests or for legacy identity)
this is great, thanks for putting together. should we wait for this to be merged and you can follow on with a PR? I am also working on the next PR in this series but that work does not conflict with yours |
|
This is great! I had left a question in the follow-on PR #3155 about the change to the pins store, which I think is the internal name for the Package.resolved? If so, I wonder if there is something we can do to make the experience better for old toolchains (e.g. 5.4) encountering the new keys. I agree with the change, just thinking about the ergonomics of the change. |
@tomerd It's as you like it. I'm happy to submit a follow-up PR if that makes things easier for you. |
|
@swift-ci please smoke test macOS Platform |
|
closing in favor of #3244 |
motivation: improve correctness by using identity more broadly throughout the code
changes:
❗ this the the first part in a series of PRs, once this is merged and "baked" will continue with the next steps