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.

Sign up for GitHub

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

Jump to bottom

Refactor RestoreCommand.ExecuteAsync() #6162

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

Refactor RestoreCommand.ExecuteAsync() #6162

wants to merge 7 commits into from
+327 −242

Conversation

Nigusu-Allehu
Copy link
Contributor

@Nigusu-Allehu Nigusu-Allehu commented Nov 25, 2024 •
edited
Loading

Bug

Fixes: NuGet/Home#13960

Description

The RestoreCommand.ExecuteAsync() method is long and complicated to read. This PR refactors it into multiple methods.

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@Nigusu-Allehu Nigusu-Allehu requested a review from a team as a code owner November 25, 2024 21:59
@Nigusu-Allehu Nigusu-Allehu self-assigned this Nov 25, 2024
@nkolev92
Copy link
Member

I think your rebase removed 0029325, which is why your tests are failing.

nkolev92
nkolev92 reviewed Nov 26, 2024
View reviewed changes
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.

Generally looks good.
My feedback is primarily stylistic, but then again, refactoring is mostly that, style changes.

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs Show resolved Hide resolved
src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs Show resolved Hide resolved
src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs Outdated Show resolved Hide resolved
jgonz120
jgonz120 previously approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@jgonz120 jgonz120 left a comment

Choose a reason for hiding this comment

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

lgtm

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs Outdated Show resolved Hide resolved
jeffkl
jeffkl previously approved these changes Dec 2, 2024
View reviewed changes
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.

Looks like you'll just need to remove an extra newline per Jonatan's comment.

@Nigusu-Allehu Nigusu-Allehu dismissed stale reviews from jeffkl and jgonz120 via c8bf25a December 3, 2024 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffkl jeffkl jeffkl left review comments

@jgonz120 jgonz120 jgonz120 left review comments

@nkolev92 nkolev92 Awaiting requested review from nkolev92

At least 1 approving review is required to merge this pull request.

Assignees

@Nigusu-Allehu Nigusu-Allehu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor RestoreCommand.ExecuteAsync()
4 participants
@Nigusu-Allehu @nkolev92 @jgonz120 @jeffkl