Skip to content

added CA1839 rule documentation#23101

Closed
MeikTranel wants to merge 3 commits intodotnet:mainfrom
MeikTranel:NewRule-CA1839
Closed

added CA1839 rule documentation#23101
MeikTranel wants to merge 3 commits intodotnet:mainfrom
MeikTranel:NewRule-CA1839

Conversation

@MeikTranel
Copy link
Contributor

Summary

Adds required documentation for rule CA1839.

Required documentation for dotnet/runtime#47180 (PR: https://github.com/dotnet/roslyn-analyzers/pull/4908/files)

@Youssef1313
Copy link
Member

@gewarren Please hold on merging until roslyn-analyzers PR is merged first.

Comment on lines +33 to +39
```csharp
public bool ContainsLetterI()
{
var testString = "I am a test string.";
return testString.Contains("I");
}
```
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the vb equivalent and dev_langs metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course - question is if I have to use the file reference syntax into the snippets folder or if there is a way to side-by-side the code examples inline of the markdown file since the snippet is so short.

Copy link
Member

Choose a reason for hiding this comment

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

@MeikTranel The benefit of a snippet file is having build verification for the code in it.

I'd wait for @gewarren to tell if you have to use it or it's okay to keep the code in markdown.

Copy link
Contributor

Choose a reason for hiding this comment

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

When it's this short I'm okay with putting it inline. You can just stack the snippets in the markdown file. Only one will be displayed depending on the user's selected language.

Copy link
Member

@AraHaan AraHaan Mar 5, 2021

Choose a reason for hiding this comment

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

I would do something like this:

public bool ContainsLetterI()
{
    var testString = "I am a test string.";
    return testString.Contains("I"); // violation of CA1839
}

// solution for .NET Standard 2.1, .NET Core 3.1, and .NET 5 or newer: 
public bool ContainsLetterI()
{
    var testString = "I am a test string.";
    return testString.Contains('I'); // fix for CA1839.
}

// fix for any Target Framework older than .NET Standard 2.1, and .NET Core 3.1 (such as targeting .NET Framework for example).
public bool ContainsLetterI()
{
    var testString = "I am a test string.";
    return testString.IndexOf('I') > -1; // fix for CA1839.
}
Public Function ContainsLetterI as Boolean
    Dim testString As String = "I am a test string."
    Return testString.Contains("I") ' violation of CA1839
End Function

' solution for .NET Standard 2.1, .NET Core 3.1, and .NET 5 or newer:
Public Function ContainsLetterI as Boolean
    Dim testString As String = "I am a test string."
    Return testString.Contains("I"c) ' fix for CA1839.
End Function


' fix for any Target Framework older than .NET Standard 2.1, and .NET Core 3.1 (such as targeting .NET Framework for example).
Public Function ContainsLetterI as Boolean
    Dim testString As String = "I am a test string."
    Return testString.IndexOf("I"c) > -1 ' fix for CA1839.
End Function

Also I would recommend trying to make an F# version as well too for those who use F#.

Copy link
Member

Choose a reason for hiding this comment

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

@AraHaan I don't think CAxxxx rules are applicable to F#

Copy link
Member

Choose a reason for hiding this comment

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

Might not but only one way to find out by testing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean "by testing that"? Analyzers have applicable languages as seen in the other PR you commented on - this is a "roslyn" analyzers rule - last i checked fsharp doesnt use roslyn. If you want this to be supported in fsharp - please ask the fsharp people for support.
As for the requested change i would recommend you open a separate issue for this. This is guidance that we don't offer anywhere else and the underlying issue is definitely not unique to this rule. Its a simple rule and the documentation should be simple as well imho.

@MeikTranel MeikTranel requested a review from a team as a code owner March 4, 2021 21:14
Comment on lines +33 to +39
```csharp
public bool ContainsLetterI()
{
var testString = "I am a test string.";
return testString.Contains("I");
}
```
Copy link
Member

@AraHaan AraHaan Mar 5, 2021

Choose a reason for hiding this comment

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

I would do something like this:

public bool ContainsLetterI()
{
    var testString = "I am a test string.";
    return testString.Contains("I"); // violation of CA1839
}

// solution for .NET Standard 2.1, .NET Core 3.1, and .NET 5 or newer: 
public bool ContainsLetterI()
{
    var testString = "I am a test string.";
    return testString.Contains('I'); // fix for CA1839.
}

// fix for any Target Framework older than .NET Standard 2.1, and .NET Core 3.1 (such as targeting .NET Framework for example).
public bool ContainsLetterI()
{
    var testString = "I am a test string.";
    return testString.IndexOf('I') > -1; // fix for CA1839.
}
Public Function ContainsLetterI as Boolean
    Dim testString As String = "I am a test string."
    Return testString.Contains("I") ' violation of CA1839
End Function

' solution for .NET Standard 2.1, .NET Core 3.1, and .NET 5 or newer:
Public Function ContainsLetterI as Boolean
    Dim testString As String = "I am a test string."
    Return testString.Contains("I"c) ' fix for CA1839.
End Function


' fix for any Target Framework older than .NET Standard 2.1, and .NET Core 3.1 (such as targeting .NET Framework for example).
Public Function ContainsLetterI as Boolean
    Dim testString As String = "I am a test string."
    Return testString.IndexOf("I"c) > -1 ' fix for CA1839.
End Function

Also I would recommend trying to make an F# version as well too for those who use F#.

@gewarren gewarren added the 🚧 Hold for related PR Indicates a PR can only be merged when other related PRs are merged (see comments for links) label Mar 5, 2021
Base automatically changed from master to main March 5, 2021 23:33
Base automatically changed from main to master March 17, 2021 17:05
Base automatically changed from master to main March 17, 2021 17:07
@MeikTranel MeikTranel closed this Mar 17, 2021
@MeikTranel MeikTranel deleted the NewRule-CA1839 branch March 17, 2021 22:52
@MeikTranel MeikTranel restored the NewRule-CA1839 branch March 17, 2021 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🚧 Hold for related PR Indicates a PR can only be merged when other related PRs are merged (see comments for links) dotnet-fundamentals/svc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants