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

Extract local functions to make project updating simpler to understand. #74878

Merged
merged 5 commits into from
Aug 23, 2024

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner August 23, 2024 17:51
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 23, 2024
Id, solutionChanges.Solution.AddAnalyzerReferences(Id, _analyzersAddedInBatch));
}
projectUpdateState = UpdateAnalyzerReferences(
Id, solutionChanges, projectUpdateState, _analyzersRemovedInBatch, _analyzersAddedInBatch);
Copy link
Member Author

Choose a reason for hiding this comment

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

Three simple helpers now: Update metadata references. Update project references. Update analyzer references.

@@ -709,6 +632,118 @@ await _projectSystemProjectFactory.ApplyBatchChangeToWorkspaceMaybeAsync(useAsyn
if (hasAnalyzerChanges)
_projectSystemProjectFactory.Workspace.EnqueueUpdateSourceGeneratorVersion(projectId: null, forceRegeneration: true);
}

static ProjectUpdateState UpdateMetadataReferences(
Project projectBeforeMutation,
Copy link
Member Author

Choose a reason for hiding this comment

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

this guy is interesting in that we pass in the prior project state. that's needed as it is used to find teh MetadataRef instance associated with teh file path we want to remove.

else
{
// TODO: find a cleaner way to fetch this
var metadataReference = _projectSystemProjectFactory.Workspace.CurrentSolution.GetRequiredProject(Id).MetadataReferences
Copy link
Member Author

Choose a reason for hiding this comment

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

the prior logic seems terrible/broken. We were accessing the mutable current soltuion of the workspace to make this decisino? That just seems never good. I changes to pass in the current 'immutable project state' that we're performing the mutation on as that seems sane and reasonavble.

// TODO: find a cleaner way to fetch this
var metadataReference = projectBeforeMutation.MetadataReferences
.OfType<PortableExecutableReference>()
.Single(m => m.FilePath == path && m.Properties == properties);
Copy link
Member Author

Choose a reason for hiding this comment

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

note: this logic seems odd to me. doing a == properties check here seems unnecessary and probably wrong too. you can't add the same filepath with different properties afaict. but i'm not changing that now. it just seems wonky.

List<ProjectReference> projectReferencesRemovedInBatch,
List<ProjectReference> _projectReferencesAddedInBatch)
{
// Project reference adding...
Copy link
Member Author

Choose a reason for hiding this comment

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

note: project references swap the normal order. instead of remove/add, it's add/remove. I wasn't certain if it was safe to change that, so i kept it as is.

@CyrusNajmabadi
Copy link
Member Author

@jasonmalinowski @ToddGrun @dibarbet ptal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants