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

Add support for restoring slnf file from command line #4790

Merged
merged 4 commits into from
Nov 8, 2022
Merged

Add support for restoring slnf file from command line #4790

merged 4 commits into from
Nov 8, 2022

Conversation

kvpt
Copy link
Contributor

@kvpt kvpt commented Sep 9, 2022

Bug

Fixes:
NuGet/Home#10809

Regression? Last working version:

Description

Add support for restoring slnf file from command line

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@kvpt kvpt requested a review from a team as a code owner September 9, 2022 23:45
@ghost ghost added the Community PRs created by someone not in the NuGet team label Sep 9, 2022
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 this contributing @kvpt

Given that we're using reflection for an internal method, I've pinged some msbuild folks to see if they have any suggestions.

src/NuGet.Clients/NuGet.CommandLine/Common/Solution.cs Outdated Show resolved Hide resolved
@@ -327,6 +327,55 @@ public void RestoreCommand_FromSolutionFile(string configFileName)
}
}

[Theory]
[InlineData("packages.config")]
Copy link
Contributor

@erdembayar erdembayar Sep 23, 2022

Choose a reason for hiding this comment

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

Does this change work with packagesreference type project too? If yes, then we may need another test for that one too.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's super relevant.

This is about choosing which projects to restore which happens prior to any notion of packages.config vs PackageReference.

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.

Apologies for the delayed review.

src/NuGet.Clients/NuGet.CommandLine/Common/Solution.cs Outdated Show resolved Hide resolved
@@ -327,6 +327,55 @@ public void RestoreCommand_FromSolutionFile(string configFileName)
}
}

[Theory]
[InlineData("packages.config")]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's super relevant.

This is about choosing which projects to restore which happens prior to any notion of packages.config vs PackageReference.

src/NuGet.Clients/NuGet.CommandLine/Common/Solution.cs Outdated Show resolved Hide resolved
Assert.False(File.Exists(packageFileB));
}

using (var pathContext = new SimpleTestPathContext())
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to use a completely separate bath context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is to isolate the two runs because in the first we only want the package A to be restored and in the second the package B. If we only use a unique context the package A will be already restored and the second assert will fail.
The use of the second context was the simplest way that I thought to reset the state of the context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just make it theory and pass true/false to decide which slnf file to restore.

Copy link
Member

Choose a reason for hiding this comment

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

It's ok to have the previous state, as long as we account for that.

Projects having a different package covers for that. We shouldn't need the separate working directories.

@nkolev92
Copy link
Member

nkolev92 commented Nov 5, 2022

Apologies for the delay @kvpt.
@NuGet/nuget-client I'm planning on merging this next Monday. If you have any stylistic feedback, we can handle it in a future PR. For example, I'm going to tweak the test after merging.

@kvpt
Copy link
Contributor Author

kvpt commented Nov 5, 2022

No problem, I myself didn't have time lately to process the latest feedbacks.
And to be honest I didn't quite understand the changes requested for the tests.
So if you can tweak them after merging go ahead.
Thank you.

@nkolev92
Copy link
Member

nkolev92 commented Nov 5, 2022

No problem, I myself didn't have time lately to process the latest feedbacks. And to be honest I didn't quite understand the changes requested for the tests. So if you can tweak them after merging go ahead. Thank you.

No worries.
Thank you for your contribution! You definitely all did the hard work.

@nkolev92 nkolev92 merged commit d505cd8 into NuGet:dev Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants