Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update SA1004 to allow doc comments to start with the in, out and ref keywords #3836

Conversation

bjornhellander
Copy link
Contributor

Fixes #3817

@bjornhellander bjornhellander force-pushed the feature/sa1004-param-modifier-3817 branch from 64bf2b4 to d3c8af2 Compare May 11, 2024 07:30
Copy link

codecov bot commented May 11, 2024

Codecov Report

Attention: Patch coverage is 95.45455% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 97.45%. Comparing base (17b613d) to head (2a93406).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3836      +/-   ##
==========================================
+ Coverage   94.38%   97.45%   +3.07%     
==========================================
  Files         925      926       +1     
  Lines      110065   110136      +71     
  Branches     3307     3311       +4     
==========================================
+ Hits       103881   107332    +3451     
+ Misses       5174     1837    -3337     
+ Partials     1010      967      -43     

@bjornhellander bjornhellander force-pushed the feature/sa1004-param-modifier-3817 branch from d3c8af2 to a1909e0 Compare May 11, 2024 08:16
string testCode = $@"
/// <summary>
/// Description of some remarks that refer to a method: <see cref=""SomeMethod(int, int,
/// {keyword} string)""/>.
Copy link
Member

Choose a reason for hiding this comment

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

Do we also need to check this wrapping?

/// <see cref="SomeMethod(int, int, ref
/// readonly string)"/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, yes. I realized the same thing when I implemented this, but it seemed like a very rare case. Wait until someone actually reports it? :-) It won't take long if you think it is worth the time, but at that time I just skipped it.

Copy link
Member

Choose a reason for hiding this comment

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

To me it seems similar to the current report. I think most users would not wrap a comment in the middle of a cref attribute, but automatic word wrapping tools can do it.

Copy link
Contributor Author

@bjornhellander bjornhellander May 22, 2024

Choose a reason for hiding this comment

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

No problem. Added.

@sharwell sharwell merged commit ac00d09 into DotNetAnalyzers:master May 22, 2024
19 checks passed
@sharwell sharwell added this to the 1.2-beta.next milestone May 22, 2024
@bjornhellander bjornhellander deleted the feature/sa1004-param-modifier-3817 branch May 23, 2024 14:14
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
2 participants