Skip to content
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

Revised proposal for Adding dotnet CLI support for projects onboarded to CPM (Take 3) #11915

Merged
merged 20 commits into from
Jul 7, 2022

Conversation

kartheekp-ms
Copy link
Contributor

Tracking issue #11858

Rendered Design Spec

Proposal for 11807

Here is what changed when compared to the initial proposal:

  1. Modified spec template as per https://github.com/NuGet/Home/blob/dev/meta/template.md
  2. We identified various scenarios while brainstorming edge cases that are not covered in previous pull requests.

@kartheekp-ms kartheekp-ms requested a review from a team as a code owner June 23, 2022 20:19
Copy link
Contributor

@jeffkl jeffkl left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work involved in getting this spec'd out!

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Thanks for updating this!

I think the spec still suggests some scenarios are in scope, but realistically only the first 4 are.

| 1 | ❌ | ❌ | ❌ | ❌ | Add `PackageReference` to the project file. Add `PackageVersion` to the `Directory.Packages.Props` file. Use latest version from the package sources. | ✔️ |
| 2 | ❌ | ❌ | ❌ | ✔️ | Add `PackageReference` to the project file. Add `PackageVersion` to the `Directory.Packages.Props` file. Use version specified in the commandline. | ✔️ |
| 3 | ❌ | ❌ | ✔️ | ❌ | Add `PackageReference` to the project file. No changes to the `Directory.Packages.Props` file. Basically we are reusing the version defined centrally for this package. | ✔️ |
| 4 | ❌ | ❌ | ✔️ | ✔️ | Add `PackageReference` to the project file. Update `PackageVersion` in the `Directory.Packages.Props` file with the version specified in the commandline. | ✔️ |
Copy link
Member

Choose a reason for hiding this comment

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

This creates an experience gap which is the dotnet add package equivalent for this issue: #11912.

@jeffkl Do you want to use the same issue for any potential updates there or should we create a dotnet add package specific one?

Copy link
Contributor Author

@kartheekp-ms kartheekp-ms Jun 28, 2022

Choose a reason for hiding this comment

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

I guess the code path is shared for VS and dotnet.exe. Hence once we address #11912 then it would impact dotnet add package also. Please correct if my understanding is incorrect.

https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs#L331

Copy link
Member

Choose a reason for hiding this comment

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

Not all the code is shared for that actually.
There's a lot of NuGetPackageManager usage in VS.
VS can also update multiple projects at a time.

@pragnya17
Copy link
Contributor

@jeffkl @nkolev92 @kartheekp-ms I have pushed the changes we discussed at our spec review meeting. Please let me know if it all looks good!

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

LGTM.

proposed/2022/cpm-dotnet-cli-add-package-support.md Outdated Show resolved Hide resolved
Co-authored-by: Nikolche Kolev <nikolev@microsoft.com>
@kartheekp-ms kartheekp-ms merged commit 97513da into dev Jul 7, 2022
@kartheekp-ms kartheekp-ms deleted the dev-kartheekp-ms-cpmaddpackagesupport-revised branch July 7, 2022 03:35
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.

None yet

5 participants