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

Better diagnostic location for types #5402

Merged
merged 3 commits into from
Aug 30, 2021

Conversation

pgovind
Copy link
Contributor

@pgovind pgovind commented Aug 24, 2021

I've implemented better diagnostic locations for types implementing preview interfaces. Instead of the diagnostic appearing on the type itself, it now appears on the interface in the interface list. I think this is a good starting point to get feedback first, and if I get sign off here, I can merge this PR and work on improving the diagnostic location for methods and other types with the same approach.

@pgovind pgovind requested a review from a team as a code owner August 24, 2021 22:29
Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, this API could be useful for other analyzers which need to flag the implemented interfaces, would you consider it?

Comment on lines +213 to +215
public partial {classOrStruct} Zoo";

csInput += @": NonPreviewInterface, {|#0:IFoo|}
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Here and all other tests:
Not sure why added this string concatenation, do you think it looks better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you think it looks better?

Yup. String interpolation uses {} and the diagnostic markers also use {| |}. Then I needed to change the code blocks using { to {{ to construct csInput, so I decided to just split it at an appropriate place. Is this harder to read though? If it is, then I'll change it in the next PR

@pgovind
Copy link
Contributor Author

pgovind commented Aug 30, 2021

I addressed @buyaa-n's comments. Just waiting for sign off from @jmarolf or somebody else from @dotnet/roslyn-analysis

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #5402 (2c78fa5) into release/6.0.1xx (3f186ae) will decrease coverage by 0.01%.
The diff coverage is 73.71%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5402      +/-   ##
===================================================
- Coverage            95.56%   95.54%   -0.02%     
===================================================
  Files                 1257     1259       +2     
  Lines               289065   289252     +187     
  Branches             17368    17377       +9     
===================================================
+ Hits                276244   276379     +135     
- Misses               10500    10540      +40     
- Partials              2321     2333      +12     

@pgovind pgovind merged commit f419533 into dotnet:release/6.0.1xx Aug 30, 2021
@pgovind
Copy link
Contributor Author

pgovind commented Aug 30, 2021

Thanks for the reviews @buyaa-n and @jmarolf!!!

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

Successfully merging this pull request may close these issues.

None yet

3 participants