Fix discrepancies with usage of package display name/package identity in swift package edit command
#7941
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
swift package edit was reliant on the user-passed packageName argument to be able to both find the dependency (which needs the identity) as well as to match the manifest display name. Default to treating the user-passed argument as a package identity, and fix the comparisons made to the manifest by instead assuring that the canonical locations for the manifest and the dependency match. The display name was intended to be deprecated.
plemarquand
requested changes
Sep 6, 2024
Keep packageName in the Edit command so it does not introduce a breaking change; add validate method to propagate the value of the packageName to packageIdentity.
bripeticca
requested review from
bnbarham,
MaxDesiatov,
jakepetroules,
francescomikulis,
xedin,
dschaefer2,
shawnhyam and
owenv
as code owners
Update the Unedit command as well.
plemarquand
reviewed
Sep 9, 2024
plemarquand
reviewed
Sep 10, 2024
|
@swift-ci please test |
|
@swift-ci please test macOS platform |
|
FYI I'll be writing a forum post about this change before I merge. |
plemarquand
approved these changes
Sep 14, 2024
| @@ -32,16 +32,16 @@ extension SwiftPackageCommand { | |||
| @Option(help: "Create or use the checkout at this path") | |||
| var path: AbsolutePath? | |||
|
|
|||
| @Argument(help: "The name of the package to edit") | |||
| var packageName: String | |||
| @Argument(help: "The identity of the package to edit.") | |||
There was a problem hiding this comment.
Nit: none of the other help strings have a period
There was a problem hiding this comment.
Good catch, thank you!
| @@ -61,15 +61,15 @@ extension SwiftPackageCommand { | |||
| help: "Unedit the package even if it has uncommitted and unpushed changes") | |||
| var shouldForceRemove: Bool = false | |||
|
|
|||
| @Argument(help: "The name of the package to unedit") | |||
| var packageName: String | |||
| @Argument(help: "The identity of the package to unedit.") | |||
|
@swift-ci please test |
|
@swift-ci please test windows platform |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7931
swift package editwas reliant on the user-passedpackageNameargument to be able to both find the dependency (which needs the identity) as well as to match the manifest display name.Default to treating the user-passed argument as a package identity, and fix the comparisons made to the manifest by instead assuring that the canonical locations for the manifest and the dependency match. The display name was intended to be deprecated.
Motivation:
The vscode-swift extension made use of of the package identities when listing dependencies in its UI. When a user would execute a
Use Local Versionon one of these dependencies, it would callswift package edit <package-name>under the hood, using the identity for the package-name argument. However, this would fail as theeditcommand would also treat the package name argument as the package's manifestdisplayName, which is not guaranteed to match the identity.Modifications:
Treat the user-passed argument as the package identity, and deprecate the usage of
displayNamein theeditcommand. Fix any necessary test cases to follow suite with this change.Result:
The
editcommand should now be using the package identity, and there should no longer be any discrepancies.