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

Refactor RestoreCommand.ExecuteAsync() #6162

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

Conversation

Nigusu-Allehu
Copy link
Contributor

@Nigusu-Allehu Nigusu-Allehu commented Nov 25, 2024

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.

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.

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

private async Task<IEnumerable<RestoreTargetGraph>> GenerateRestoreGraphsAsync(TelemetryActivity telemetry, RemoteWalkContext contextForProject, CancellationToken token)
{
IEnumerable<RestoreTargetGraph> graphs = null;

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out! I added the new line before the if statement for readability but happy to adjust if needed.

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.

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.

Refactor RestoreCommand.ExecuteAsync()
4 participants