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.

}
}

return null;
Copy link
Member

Choose a reason for hiding this comment

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

When is the cache file null?

Feels like it shouldn't be.
Worth double checking.

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 return null was added to ensure the method covers all possible code paths with a return statement. I modified the ExecuteAsync method to handle scenarios where this method returns null, allowing such cases to be ignored.

Previous ExecuteAsync

{
    // EvaluateNoOpAsync method logic
}

Updated ExecuteAsync

{
    if (EvaluateNoOpAsync() is null)
    {
        continue;
    }
    else
    {
        return noOpResult;
    }
}

EvaluateNoOpAsync

EvaluateNoOpAsync()
{
    // Same logic

    // Ensure all paths return
    return null;
}

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

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