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

CA1700: Do not name enum values 'Reserved' #3365

Merged
merged 7 commits into from
Apr 6, 2020
Merged

Conversation

Evangelink
Copy link
Member

Fix #440

@Evangelink Evangelink requested a review from a team as a code owner March 10, 2020 17:34
<value>This rule assumes that an enumeration member that has a name that contains "reserved" is not currently used but is a placeholder to be renamed or removed in a future version. Renaming or removing a member is a breaking change.</value>
</data>
<data name="DoNotNameEnumValuesReservedMessage" xml:space="preserve">
<value>If '{0}.{1}' is not used in the current implementation, remove it. Otherwise give it a meaningful name.</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.

Message is copied from legacy fxcop

@@ -1370,4 +1370,13 @@
<data name="EnumShouldNotHaveDuplicatedValuesTitle" xml:space="preserve">
<value>Enums values should not be duplicated</value>
</data>
<data name="DoNotNameEnumValuesReservedDescription" xml:space="preserve">
<value>This rule assumes that an enumeration member that has a name that contains "reserved" is not currently used but is a placeholder to be renamed or removed in a future version. Renaming or removing a member is a breaking change.</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.

Copied from first part of the rule description.

@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #3365 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #3365    +/-   ##
========================================
  Coverage   95.30%   95.30%            
========================================
  Files        1025     1027     +2     
  Lines      232931   233052   +121     
  Branches    15052    15068    +16     
========================================
+ Hits       221990   222107   +117     
- Misses       9278     9282     +4     
  Partials     1663     1663            

s_localizableTitle,
s_localizableMessageRule,
DiagnosticCategory.Naming,
RuleLevel.IdeHidden_BulkConfigurable,
Copy link
Member Author

Choose a reason for hiding this comment

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

What shall be the default level?

@mavasani
Copy link
Contributor

FYI: You will need to fix a new RS diagnostic once #3444 goes in.

{
var field = (IFieldSymbol)context.Symbol;

// FxCop compat: only analyze externally visible symbols by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for handling this!

}


if (field.ContainingType?.TypeKind == TypeKind.Enum &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Might be cheaper to do this checks before the MatchesConfiguredVisibility check...



if (field.ContainingType?.TypeKind == TypeKind.Enum &&
field.Name.Contains("Reserved"))
Copy link
Contributor

Choose a reason for hiding this comment

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

FxCop rule implementation uses WordParser, which has been ported to this repo, so we should also match it:

if (!WordParser.ContainsWord(member.Name.Name, WordParserOptions.SplitCompoundWords, "reserved"))
           return null;

Copy link
Contributor

Choose a reason for hiding this comment

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

You can likely write a test by using a field name such as Preserved or such.

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.

Overall, LGTM - should be good to merge once the suggestions are addressed. Thanks!

@Evangelink Evangelink requested a review from mavasani April 6, 2020 12:46
}

// FxCop compat: only analyze externally visible symbols by default.
if (!field.MatchesConfiguredVisibility(context.Options, Rule, context.CancellationToken))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the analyzer configuration document that this rule supports api_surface.

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.

Should be good to merge once the analyzer configuration document is updated. Thanks!

@mavasani mavasani merged commit 8e82353 into dotnet:master Apr 6, 2020
@Evangelink Evangelink deleted the CA1700 branch May 12, 2020 09:42
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.

Port FxCop rule CA1700: DoNotNameEnumValuesReserved
2 participants