-
Notifications
You must be signed in to change notification settings - Fork 747
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 SourceFileGrouping to remove provider logic #13522
Conversation
Test this change out locally with the following install scripts (Action run 8165730326) VSCode
Azure CLI
|
23eb5c1
to
4a79c0f
Compare
Please make sure #13495 gets merged first, in case there are conflicts. |
Will do! And I'm sure there will be :( |
27b4fe2
to
7d5d4db
Compare
7d5d4db
to
5472621
Compare
|
||
if (!uriResult.IsSuccess(out var artifactUri)) | ||
var resolutionInfo = GetArtifactRestoreResult(file, restorable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to evaluate GetArtifactRestoreResult(file, restorable);
once above the if-clause and use its result rather than calling the function in two seaparate places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why this is beneficial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just my opinion, but I think there are 2 main benefits:
- As a reader it makes it clear that the artifact restore result doesn't depend on the restorable type, and that the restore result is terminal
- It reduces the chance that one will change but not the other in future refactorings
These may not be strong enough justification, can you share your thoughts on the drawbacks of my suggestion and the benefits of keeping it as is?
LGTM, ship it! 🚀 |
Hey @anthony-c-martin , can this wait for #13537? |
Sure, happy to wait |
I felt like the logic in the
SourceFileGroupingBuilder
was becoming overly complex, with the introduction of concerns about how to parse and interpret provider declarations. This refactor moves provider interpretation logic intoDeclarationVisitor
, and keeps the responsibility of theSourceFileGroupingBuilder
focused on obtaining the full set of source files + files that need to be restored from the registry.Microsoft Reviewers: Open in CodeFlow