Skip to content

Update all resources in RoslynDiagnosticAnalyzerResources #3543

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

Merged
merged 1 commit into from
Apr 27, 2020

Conversation

Evangelink
Copy link
Member

No description provided.

@Evangelink Evangelink requested a review from a team as a code owner April 23, 2020 08:14
@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #3543 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3543   +/-   ##
=======================================
  Coverage   95.09%   95.09%           
=======================================
  Files        1053     1053           
  Lines      237724   237724           
  Branches    15449    15449           
=======================================
+ Hits       226065   226069    +4     
+ Misses       9943     9939    -4     
  Partials     1716     1716           

</data>
<data name="DoNotInvokeDiagnosticDescriptorDescription" xml:space="preserve">
<value>Accessing the Descriptor property of Diagnostic in compiler layer leads to unnecessary string allocations for fields of the descriptor that are not utilized in command line compilation. Hence, you should avoid accessing the Descriptor of the compiler diagnostics here. Instead you should directly access these properties off the Diagnostic type.</value>
<value>Accessing the 'Descriptor' property of 'Diagnostic' in compiler layer leads to unnecessary string allocations for fields of the descriptor that are not utilized in command line compilation. Hence, you should avoid accessing the 'Descriptor' of the compiler diagnostics here. Instead you should directly access these properties off the 'Diagnostic' type.</value>
</data>
<data name="DoNotInvokeDiagnosticDescriptorMessage" xml:space="preserve">
<value>Do not invoke property '{0}' on type '{1}', instead directly access the required member{2} on '{1}'</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<value>Do not invoke property '{0}' on type '{1}', instead directly access the required member{2} on '{1}'</value>
<value>Do not invoke property '{0}' on type '{1}', instead directly access the required member '{2}' on '{1}'</value>

Copy link
Member Author

Choose a reason for hiding this comment

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

We shouldn't change this (or at least not in this PR), the code is taking care of adding the space + wrapping in single quotes when needed: https://github.com/dotnet/roslyn-analyzers/blob/master/src/Roslyn.Diagnostics.Analyzers/Core/DoNotInvokeDiagnosticDescriptor.cs#L64

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah got you. Can you file a separate issue for fixing this up in a separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

LGTM. Once you apply the additional suggestion and update xlf files, I can merge. Thanks!

@Evangelink
Copy link
Member Author

@mavasani I have replied to your comment, the resource is expected to be as it is currently. I can either keep it as-is, fix it and fix code here or create a ticket and do a separate PR to fix this.

@mavasani mavasani merged commit 9ac40dc into dotnet:master Apr 27, 2020
@Evangelink Evangelink deleted the roslyn-resx branch April 27, 2020 16:05
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

2 participants