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

Code fix for using statements #650

Merged
merged 1 commit into from
Sep 9, 2015

Conversation


var firstPartName = ((IdentifierNameSyntax)firstPart).Identifier.ValueText;

if (string.CompareOrdinal(firstPartName, "System") == 0)
Copy link
Member

Choose a reason for hiding this comment

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

You could use "System".Equals(firstPartName, StringComparison.Ordinal) her instead

Choose a reason for hiding this comment

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

nameof(System)

2015-05-05 11:57 GMT+02:00 Dennis Fischer notifications@github.com:

In
StyleCop.Analyzers/StyleCop.Analyzers/OrderingRules/UsingCodeFixProvider.cs
#650 (comment)
:

  •            else if (!usingDirective.StaticKeyword.IsKind(SyntaxKind.None))
    
  •            {
    
  •                staticImports.Add(usingDirective);
    
  •            }
    
  •            else
    
  •            {
    
  •                var firstPart = usingDirective.Name;
    
  •                while (firstPart is QualifiedNameSyntax)
    
  •                {
    
  •                    firstPart = ((QualifiedNameSyntax)firstPart).Left;
    
  •                }
    
  •                var firstPartName = ((IdentifierNameSyntax)firstPart).Identifier.ValueText;
    
  •                if (string.CompareOrdinal(firstPartName, "System") == 0)
    

You could use "System".Equals(firstPartName, StringComparison.Ordinal)
her instead


Reply to this email directly or view it on GitHub
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/pull/650/files#r29655896
.

Copy link
Member

Choose a reason for hiding this comment

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

I didnt even know that you could use nameof with namespaces!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the code to use nameof(System)

@vweijsters
Copy link
Contributor Author

Updated to reflect the current state

@sharwell
Copy link
Member

I'm treating this as blocked prior to implementing the rest of the using diagnostics (#288, #289, #290).

{
foreach (Diagnostic diagnostic in context.Diagnostics.Where(d => FixableDiagnostics.Contains(d.Id)))
{
context.RegisterCodeFix(CodeAction.Create("Reorder using statements", token => GetTransformedDocumentAsync(context.Document, diagnostic, token)), diagnostic);
Copy link
Member

Choose a reason for hiding this comment

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

this should be updated with equivalence key

@Noryoko
Copy link
Contributor

Noryoko commented Aug 23, 2015

❓ Can you update the unit tests for the diagnostics from DiagnosticVerifier to CodeFixVerifier?

❓ In the case of directive trivia surrounding using statements outside of a namespace how will these be handled?

Moving the using statement into the namespace in the following example will cause a compiler error. This is handled correctly by the diagnostic. Pointing it out as a heads up.

using System.Reflection;

[assembly: AssemblyVersion("1.0.0.0")]

namespace TestNamespace { }

@sharwell
Copy link
Member

@vweijsters I'd love to include this, but first I want to make sure it accounts for a couple of things to make sure we don't break users' code.

  1. When SA1200 is suppressed, the code fix should not attempt to move the using declarations inside a namespace.
  2. When the file contains items declared outside a namespace (such as @Noryoko's example), or when the file contains more than one namespace, the code fix should not attempt to move using declarations inside a namespace.
  3. The code fix should account for Order of using directives should reset at preprocessor directives #1161 when reordering directives. In addition, the code fix should not attempt to move using directives inside a namespace if the group contained unbalanced preprocessor directives.

@vweijsters
Copy link
Contributor Author

I've been busy the last couple of days. This one is first on my list for the coming week

@sharwell sharwell added this to the 1.0.0 Beta 10 milestone Aug 28, 2015
@sharwell
Copy link
Member

Thanks for the update. 😄 I marked it as in progress with a tentative milestone of Beta 10.

@vweijsters
Copy link
Contributor Author

Updated with all remarks above. Also rebased/squashed.

{
}
}";

DiagnosticResult[] expectedForCompilationUnit =
{
this.CSharpDiagnostic().WithLocation(1, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Single diagnostics can be:
var expected = this.CSharpDiagnostic().WithLocation(1, 1);


private static async Task<Document> GetTransformedDocumentAsync(Document document, Diagnostic diagnostic, CancellationToken cancellationToken)
{
var syntaxRoot = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Could syntaxRoot be passed in?

@Noryoko
Copy link
Contributor

Noryoko commented Sep 8, 2015

Can you add a test that includes a file header?

@sharwell sharwell merged commit e447421 into DotNetAnalyzers:master Sep 9, 2015
@sharwell
Copy link
Member

sharwell commented Sep 9, 2015

I'll send a follow-up pull request to address the final feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment