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

Addressing PR comments for Transitives in Solution PM UI #6064

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

martinrrm
Copy link
Contributor

@martinrrm martinrrm commented Sep 27, 2024

Bug

Fixes: NuGet/Home#13216

Description

Addressing feedback and comments from the original pull request for the Transitive Packages in Solution PM UI feature: #6044 (comment).

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • [ ] Added tests No tests needed, just addressing some minor fixes
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

Copy link
Contributor

@jebriede jebriede left a comment

Choose a reason for hiding this comment

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

Please link the GitHub issue (you can use the original feature), add a description, and check the boxes on the PR.

@@ -237,7 +237,7 @@ public async Task SearchAsync_WhenMultitargetingAndEmptySearch_ReturnsOnePackage
e => Assert.Equal(new PackageIdentity("packageC", new NuGetVersion("2.0.0")), e.Identity),
// Returns latest transitive package version
e => Assert.Equal(new PackageIdentity("transitivePackageC", new NuGetVersion("0.0.2")), e.Identity),
// Returns all the transtive packages that are not the latest verion
// Returns all the transitive packages that are not the latest version
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add the extra context and explanation of why we are getting all the transitive packages except the latest version, as requested in this comment: #6044 (comment)

Copy link
Contributor Author

@martinrrm martinrrm Sep 27, 2024

Choose a reason for hiding this comment

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

Oops I don't know how the commit didn't include the comment I added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is nothing special about transitive's. The same assert is being done for top-level packages.

From Donnie comment

If they are sorted, why is transitivePackageC 0.0.2 asserted before version 0.0.1 ?

Packages are sorted using SemVer so 0.0.2 is newer than 0.0.1

@martinrrm martinrrm marked this pull request as ready for review September 27, 2024 23:10
@martinrrm martinrrm requested a review from a team as a code owner September 27, 2024 23:10
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.

[Feature] Enable Transitive Dependencies for Solution-level in Visual Studio
2 participants