Skip to content

Handle FAR for global suppressions in a consistent fashion#54641

Merged
CyrusNajmabadi merged 2 commits intodotnet:mainfrom
CyrusNajmabadi:farRefactorings
Jul 7, 2021
Merged

Handle FAR for global suppressions in a consistent fashion#54641
CyrusNajmabadi merged 2 commits intodotnet:mainfrom
CyrusNajmabadi:farRefactorings

Conversation

@CyrusNajmabadi
Copy link
Contributor

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners July 6, 2021 21:35
@ghost ghost added the Area-IDE label Jul 6, 2021
Copy link
Contributor

@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


[PerformanceSensitive("https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1224834", OftenCompletesSynchronously = true)]
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
Contributor 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
Contributor 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 CyrusNajmabadi enabled auto-merge July 6, 2021 23:00
@CyrusNajmabadi
Copy link
Contributor 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