Add parameter xml documentation code fix provider [work in progress] #10520

janrapp · 2016-04-13T00:31:31Z

This is about #5611 and #7070.

Questions

I use the SyntaxExtensions GetElementKind but it's internal and in a different project. Currently I pretty much need this exact functionality. It's the only method that has XmlNameAttributeElementKind in its signature and XmlNameAttributeElementKind is public. How bad is the idea to make it part of the public API in the compiler layer?
Should we have the same CodeFixProvider for param and typeparam as I have now?
- Note: I even use the same EquivalenceKey for the resulting code action.

TODO

/** */ comments are handled incorrect! An idea and details in Quick Fix: Add parameter to documentation #5611.
I'm still working to get several occurrences fixed at once.
I need more unit tests for different things with parameters or type parametes (e.g. delegates).
Small things and // TODO comments

Marginal remarks

I copied some of the practices from the HideBaseCodeFixProvider.
Performance: I don't really know. I think that it might not be so important as this only executes if the warning appears.
In the pull request Fix casing on some resource strings #9246 the file src\Features\CSharp\Portable\CSharpFeaturesResources.resx changed which causes Visual Studio to change the ....Designer.cs (only the comments). That produced changes that don't belong here so I'll remove them before this PR becomes ready for review. Maybe someone should fix that (maybe not?). I would make a pull request if someone wants me to.
I'll squash all commits.
I'm new to programming things that have many other involved and are supposed to live a little longer. So I especially welcome critique/comments as my code may not be the result of taste but of ignorance.

Pilchie · 2016-04-13T00:56:32Z

@dotnet-bot test eta please, test vsi please

@dotnet/roslyn-ide Please review.

DustinCampbell · 2016-04-13T01:20:36Z

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

@@ -2,6 +2,7 @@ Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.CSharpCompilationOptions(
 Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.CSharpCompilationOptions(Microsoft.CodeAnalysis.OutputKind outputKind, bool reportSuppressedDiagnostics, string moduleName, string mainTypeName, string scriptClassName, System.Collections.Generic.IEnumerable<string> usings, Microsoft.CodeAnalysis.OptimizationLevel optimizationLevel, bool checkOverflow, bool allowUnsafe, string cryptoKeyContainer, string cryptoKeyFile, System.Collections.Immutable.ImmutableArray<byte> cryptoPublicKey, bool? delaySign, Microsoft.CodeAnalysis.Platform platform, Microsoft.CodeAnalysis.ReportDiagnostic generalDiagnosticOption, int warningLevel, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, Microsoft.CodeAnalysis.ReportDiagnostic>> specificDiagnosticOptions, bool concurrentBuild, bool deterministic, Microsoft.CodeAnalysis.XmlReferenceResolver xmlReferenceResolver, Microsoft.CodeAnalysis.SourceReferenceResolver sourceReferenceResolver, Microsoft.CodeAnalysis.MetadataReferenceResolver metadataReferenceResolver, Microsoft.CodeAnalysis.AssemblyIdentityComparer assemblyIdentityComparer, Microsoft.CodeAnalysis.StrongNameProvider strongNameProvider) -> void
 Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.CSharpCompilationOptions(Microsoft.CodeAnalysis.OutputKind outputKind, string moduleName, string mainTypeName, string scriptClassName, System.Collections.Generic.IEnumerable<string> usings, Microsoft.CodeAnalysis.OptimizationLevel optimizationLevel, bool checkOverflow, bool allowUnsafe, string cryptoKeyContainer, string cryptoKeyFile, System.Collections.Immutable.ImmutableArray<byte> cryptoPublicKey, bool? delaySign, Microsoft.CodeAnalysis.Platform platform, Microsoft.CodeAnalysis.ReportDiagnostic generalDiagnosticOption, int warningLevel, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, Microsoft.CodeAnalysis.ReportDiagnostic>> specificDiagnosticOptions, bool concurrentBuild, bool deterministic, Microsoft.CodeAnalysis.XmlReferenceResolver xmlReferenceResolver, Microsoft.CodeAnalysis.SourceReferenceResolver sourceReferenceResolver, Microsoft.CodeAnalysis.MetadataReferenceResolver metadataReferenceResolver, Microsoft.CodeAnalysis.AssemblyIdentityComparer assemblyIdentityComparer, Microsoft.CodeAnalysis.StrongNameProvider strongNameProvider) -> void
 Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.WithPublicSign(bool publicSign) -> Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions
+static Microsoft.CodeAnalysis.CSharp.SyntaxExtensions.GetElementKind(this Microsoft.CodeAnalysis.CSharp.Syntax.XmlNameAttributeSyntax attributeSyntax) -> Microsoft.CodeAnalysis.CSharp.Syntax.XmlNameAttributeElementKind


Making this public is going to require a much larger review. If you can avoid this changing public API surface area, it'd be helpful.

DustinCampbell · 2016-04-13T01:23:32Z

@3jr -- please take a look at #7070. We have plans for many more fixes here.

DustinCampbell · 2016-04-13T01:25:18Z

...rtable/CodeFixes/AddParameterXmlDocumentation/AddParameterXmlDocumentationCodeFixProvider.cs

+                context.RegisterCodeFix(
+                    new AddParameterXmlDocumentationCodeAction(context.Document, parameterName, adjacentAttribute,
+                        adjacentAttributeIsBefore, isTypeParameter: false),
+                    diagnostic);


Extract this to a method and share common code with the code below.

DustinCampbell · 2016-04-13T02:08:15Z

I left some initial feedback, though I'm sure others will want to take a look as well.

A couple of general notes:

I suspect the XmlNameAttributeElementKind was left public by accident and would suggest not making GetElementKind(...) public. The reason is not that I don't think it'd be a good public API. Rather, it would require an API review, which may slow down the process for this PR and possibly require more changes to properly shape the API. After all, it's likely that there are a set of APIs that should be designed and/or made public than just this one API. However, if you're passionate about this, I'm totally supportive. I just wanted to let you know that it might turn out to be a larger can of worms than you're expecting.
/** */ doc comments are something I'd suggest not spending time on. They are not well-supported at all throughout the IDE today and they have very low usage across all C# code bases.

DustinCampbell · 2016-04-13T12:54:45Z

...rtable/CodeFixes/AddParameterXmlDocumentation/AddParameterXmlDocumentationCodeFixProvider.cs

+            private bool _adjacentAttributeIsBefore;
+            private bool _isTypeParameter;
+
+            public override string Title => _title;


This can be a getter-only auto-property so you don't have to bother having a field.

janrapp · 2016-04-15T10:54:55Z

@DustinCampbell Thanks a lot for the feedback. Sorry it took me so long to respond. I'm working on a new version.

janrapp · 2016-04-15T10:55:59Z

Sorry. I didn't want to close it. I was just wondering what the "Close and Comment" meant.

Pilchie · 2016-07-31T00:49:23Z

@3jr Have you had a chance to get back to this yet? If not, would you object to someone else picking up from your starting point and continuing?

janrapp · 2016-08-05T05:40:27Z

@Pilchie Hi, I have in fact fixed all of @DustinCampbell suggestions and with that rewrote quite a bit of the code. I than started to work on a batch fixer because batch fixing doesn't work at all at the moment. I got stuck there and just kind of stopped. I don't mind if someone else works on this. I will push my new branch this evening, in case someone wan't to look at it.

janrapp · 2016-08-05T05:57:12Z

Better: #12997

janrapp · 2016-08-05T21:45:16Z

Here is my updated version. I haven't looked at it for a few weeks but if I remember correctly It worked okay:
https://github.com/3jr/roslyn/tree/AddParameterXmlDocumentationCodeFixProvider2

dnfclas added the cla-signed label Apr 13, 2016

Pilchie added the Area-IDE label Apr 13, 2016

DustinCampbell reviewed Apr 13, 2016
View reviewed changes

janrapp closed this Apr 15, 2016

janrapp reopened this Apr 15, 2016

dnfclas added the cla-already-signed label Apr 15, 2016

Add AddParameterXmlDocumentationCodeFix

b6d487e

Pilchie mentioned this pull request Jul 31, 2016

Code Style: Code fixes for xml doc comments #7070

Open

17 tasks

janrapp closed this Aug 5, 2016

janrapp mentioned this pull request Aug 6, 2016

Quick Fix: Add parameter to documentation #5611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parameter xml documentation code fix provider [work in progress] #10520

Add parameter xml documentation code fix provider [work in progress] #10520

janrapp commented Apr 13, 2016 •

edited

Loading

Pilchie commented Apr 13, 2016

DustinCampbell Apr 13, 2016

DustinCampbell commented Apr 13, 2016

DustinCampbell Apr 13, 2016

DustinCampbell commented Apr 13, 2016

DustinCampbell Apr 13, 2016

janrapp commented Apr 15, 2016

janrapp commented Apr 15, 2016 •

edited

Loading

Pilchie commented Jul 31, 2016

janrapp commented Aug 5, 2016

janrapp commented Aug 5, 2016 •

edited

Loading

janrapp commented Aug 5, 2016

Add parameter xml documentation code fix provider [work in progress] #10520

Add parameter xml documentation code fix provider [work in progress] #10520

Conversation

janrapp commented Apr 13, 2016 • edited Loading

Questions

TODO

Marginal remarks

Pilchie commented Apr 13, 2016

DustinCampbell Apr 13, 2016

Choose a reason for hiding this comment

DustinCampbell commented Apr 13, 2016

DustinCampbell Apr 13, 2016

Choose a reason for hiding this comment

DustinCampbell commented Apr 13, 2016

DustinCampbell Apr 13, 2016

Choose a reason for hiding this comment

janrapp commented Apr 15, 2016

janrapp commented Apr 15, 2016 • edited Loading

Pilchie commented Jul 31, 2016

janrapp commented Aug 5, 2016

janrapp commented Aug 5, 2016 • edited Loading

janrapp commented Aug 5, 2016

janrapp commented Apr 13, 2016 •

edited

Loading

janrapp commented Apr 15, 2016 •

edited

Loading

janrapp commented Aug 5, 2016 •

edited

Loading