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

Add code fixes for documentation diagnostics (#1) #3445

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jgefen
Copy link

@jgefen jgefen commented Jan 27, 2022

  • Add helpers

  • Add documenation fix SA1600 for most types

  • Add SA1611 codefix

  • Add SA1602 codefix

  • Add SA1618 codefix

Co-authored-by: Alon Sheffer [email protected]
Co-authored-by: Jonatan Gefen [email protected]

jgefen and others added 3 commits January 27, 2022 16:14
* Add helpers

* Add documenation fix SA1600 for most types

* Add SA1611 codefix

* Add SA1602 codefix

* Add SA1618 codefix

Co-authored-by: Alon Sheffer <[email protected]>
Co-authored-by: Jonatan Gefen <[email protected]>
@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #3445 (6332dea) into master (a821966) will decrease coverage by 0.00%.
The diff coverage is 93.26%.

❗ Current head 6332dea differs from pull request most recent head 9d2d366. Consider uploading reports for the commit 9d2d366 to get more accurate results

@@            Coverage Diff             @@
##           master    #3445      +/-   ##
==========================================
- Coverage   93.17%   93.17%   -0.01%     
==========================================
  Files        1047     1058      +11     
  Lines      112348   113868    +1520     
  Branches     3976     4039      +63     
==========================================
+ Hits       104684   106100    +1416     
- Misses       6644     6711      +67     
- Partials     1020     1057      +37     

@sharwell
Copy link
Member

📝 Keep in mind I'll be primarily evaluating this in the context of #764 (comment). It seems likely that you'll want one or more features that don't get merged here. For that, it's possible to add a custom code fix project which recognizes diagnostics produced by StyleCop Analyzers and can provide fixes beyond the scope of this project.

enum TypeName
{
/// <summary>
/// Bar.
Copy link
Member

Choose a reason for hiding this comment

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

📝 This isn't a meaningful addition, so the preference is to not automatically generate it. It's fine to generate the empty <summary> element, but the contents should not be automatically generated except in limited cases where a specific design pattern includes the documentation form.

/// <summary>
/// Foo
/// </summary>
/// <param name=""param1"">The param1.</param>
Copy link
Member

Choose a reason for hiding this comment

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

📝 The content should not be added here, except in limited cases where the content is dictated by a specific design pattern.

Suggested change
/// <param name=""param1"">The param1.</param>
/// <param name=""param1""></param>

/// Foo
/// </summary>
/// <param name=""param1"">Param 1</param>
/// <param name=""param2"">The param2.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <param name=""param2"">The param2.</param>
/// <param name=""param2""></param>

/// Foo
/// </summary>
/// <typeparam name=""TInternalRow"">The type of the internal row.</typeparam>
/// <param name=""param1"">The param1.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <param name=""param1"">The param1.</param>
/// <param name=""param1""></param>

/// </summary>
/// <param name=""param1"">The param1.</param>
/// <param name=""param2"">The param2.</param>
/// <param name=""param3"">The param3.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <param name=""param3"">The param3.</param>
/// <param name=""param3""></param>

/// Foo
/// </summary>
/// <typeparam name=""Ta"">The type of the ta.</typeparam>
/// <typeparam name=""Tb"">The type of the tb.</typeparam>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <typeparam name=""Tb"">The type of the tb.</typeparam>
/// <typeparam name=""Tb""></typeparam>

<ProjectReference
Include="..\StyleCop.Analyzers.CodeGeneration\StyleCop.Analyzers.CodeGeneration.csproj"
SetTargetFramework="TargetFramework=netstandard2.0"
OutputItemType="Analyzer"
Copy link
Member

Choose a reason for hiding this comment

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

💭 I'm guessing the change here was unintentional?

/// <param name=""param1""></param>
/// <param name=""param2""></param>
/// <param name=""param1"">The param1.</param>
/// <param name=""param2"">If true, param2.</param>
Copy link
Member

Choose a reason for hiding this comment

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

📝 The standard form for a Boolean-value property is:

<see langword="true"/> if [text]; otherwise, <see langword="false"/>.

This could be generated using a placeholder:

<see langword="true"/> if <placeholder>condition</placeholder>; otherwise, <see langword="false"/>.

Comment on lines +807 to +813
Diagnostic().WithLocation(9, 17),
Diagnostic().WithLocation(13, 16),
Diagnostic().WithLocation(18, 16),
Diagnostic().WithLocation(23, 16),
Diagnostic().WithLocation(28, 40),
Diagnostic().WithLocation(33, 25),
Diagnostic().WithLocation(38, 25),
Copy link
Member

Choose a reason for hiding this comment

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

💡 Use [|...|] syntax inside of testCode to mark the diagnostic locations

/// <summary>
/// Test class.
/// </summary>
public class TestClass
Copy link
Member

Choose a reason for hiding this comment

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

📝 Most values added in this test need to be updated according to other comments given previously.

@bjornhellander
Copy link
Contributor

I'm looking through the older pull requests in this repo. Is this something that you will continue working on, @jgefen?

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.

5 participants