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

Handle FAR for global suppressions in a consistent fashion #54641

Merged
merged 2 commits into from
Jul 7, 2021

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

Copy link
Member

@cston cston left a comment

Choose a reason for hiding this comment

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

Compiler changes LGTM.

Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

Can we add tests for this? Seems like if we're changing FAR behavior we should have some tests to cover what is consistent

Comment on lines 107 to 108
/// Finds all the documents in the provided project that contain the requested string
/// values
Copy link
Contributor

Choose a reason for hiding this comment

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

"requested string values"

What requested string values? This just finds where ContainsGlobalAttribues is true

docCommentId, findInGlobalSuppressions, cancellationToken);
}

[PerformanceSensitive("https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1224834", OftenCompletesSynchronously = true)]
Copy link
Contributor

@ryzngard ryzngard Jul 6, 2021

Choose a reason for hiding this comment

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

Unsure why this is no longer perf sensitive

protected static async ValueTask<ImmutableArray<FinderLocation>> FindReferencesInDocumentUsingIdentifierAsync(
ISymbol _,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why take a symbol if it's not used?

@ryzngard
Copy link
Contributor

ryzngard commented Jul 6, 2021

Can we add tests for this? Seems like if we're changing FAR behavior we should have some tests to cover what is consistent

Assuming this is a behavior change. With all of the FAR changes I'm not 100% sure if this is just a refactoring keeping old behavior. If it is, please add that to the PR description :)

@CyrusNajmabadi
Copy link
Member Author

Can we add tests for this? Seems like if we're changing FAR behavior we should have some tests to cover what is consistent

This area is already extensively tested. This doesn't change behavior. It just updates the implementation to follow the general patterns of hte rest of the FAR code and it removes some ugly special casing htat was happening before. I've pulled this out of a larger set of refactorings i'm making as i would like to keep the large one under control.

@CyrusNajmabadi
Copy link
Member Author

Why take a symbol if it's not used?

It will be used in a followup PR. If i remove, i need to fixup all callsties, but then i'm going to be adding it back in, so i'll just be undoing that.

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler for a tiny compiler change.

@CyrusNajmabadi CyrusNajmabadi merged commit 04c02bf into dotnet:main Jul 7, 2021
@ghost ghost added this to the Next milestone Jul 7, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the farRefactorings branch July 7, 2021 03:40
@allisonchou allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants