Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Don't add already known documents to the misc files project #10753

Merged
merged 2 commits into from
Aug 18, 2024

Conversation

davidwengier
Copy link
Member

@davidwengier davidwengier commented Aug 16, 2024

Noticed in the logs that we were double-compiling files, and it was causing issues in my "self versioned documents" branch. Separated out of that to make it easier to review.

RCA is because we now share a project manager across VS and LSP server, so we get real project information much sooner than before, which beats file watchers etc.

@davidwengier davidwengier requested a review from a team as a code owner August 16, 2024 04:04
Comment on lines 164 to 173
var potentialProjects = _projectManager.FindPotentialProjects(textDocumentPath);
foreach (var project in potentialProjects)
{
if (project.DocumentFilePaths.Contains(textDocumentPath, FilePathComparer.Instance))
{
// Already in a known project, so we don't want it in the misc files project
_logger.LogDebug($"File {textDocumentPath} is already in {project.Key} so we're not adding it to the miscellaneous files project");
return;
}
}
Copy link
Contributor

@ryzngard ryzngard Aug 16, 2024

Choose a reason for hiding this comment

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

Isn't this basically the same as the below?

Suggested change
var potentialProjects = _projectManager.FindPotentialProjects(textDocumentPath);
foreach (var project in potentialProjects)
{
if (project.DocumentFilePaths.Contains(textDocumentPath, FilePathComparer.Instance))
{
// Already in a known project, so we don't want it in the misc files project
_logger.LogDebug($"File {textDocumentPath} is already in {project.Key} so we're not adding it to the miscellaneous files project");
return;
}
}
if (_projectManager.TryResolveDocumentInAnyProject(textDocumentPath, _logger, out var project)
{
// Already in a known project, so we don't want it in the misc files project
_logger.LogDebug($"File {textDocumentPath} is already in {project.Key} so we're not adding it to the miscellaneous files project");
return;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

lol, it very well could be. That's a little embarassing :D

@davidwengier davidwengier enabled auto-merge August 18, 2024 22:50
@davidwengier davidwengier merged commit da79940 into dotnet:main Aug 18, 2024
12 checks passed
@davidwengier davidwengier deleted the WhenYouAssume branch August 18, 2024 23:53
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 18, 2024
@DustinCampbell
Copy link
Member

RCA is because we now share a project manager across VS and LSP server, so we get real project information much sooner than before, which beats file watchers etc.

This isn't quite correct. We don't share a project manager across VS and the LSP server, but we do share project information.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants