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

Instantiate ArgumentExceptions Correctly analyzer updates and fixer implementation #3500

Merged
merged 10 commits into from
Apr 26, 2020

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Apr 14, 2020

Fixes dotnet/runtime#33763

  • Updates made to analyzer:
  1. Increased severity from IdeHidden_BulkConfigurable => IdeSuggestion
  2. Don't warn when the enclosing method has no parameter
  3. Do not show multiple diagnostics for reversed arguments, it looks ugly and reporting one issue from 2 sides, only showing the diagnosing having fixer:
    image
  4. Added api surface option for configuring method visibility
  5. Do not warn if generic type parameter name used
  • Implemented the CSharp only version for fixer, please suggest how to make it more language ignostic way language agnostic fixer

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Overall, this looks pretty good to me. I'll do another pass once you can move the fixer to IOperation based language agnostic implementation. It would be fine to add just 1-2 VB code fix tests, your current coverage for C# tests looks good to me.

@codecov
Copy link

codecov bot commented Apr 17, 2020

Codecov Report

Merging #3500 into master will increase coverage by 0.00%.
The diff coverage is 98.97%.

@@           Coverage Diff            @@
##           master    #3500    +/-   ##
========================================
  Coverage   95.10%   95.10%            
========================================
  Files        1050     1053     +3     
  Lines      237440   238137   +697     
  Branches    15389    15496   +107     
========================================
+ Hits       225807   226484   +677     
- Misses       9931     9933     +2     
- Partials     1702     1720    +18     

End Sub
End Class",
GetBasicIncorrectMessageExpectedResult(4, 15, "Test", "first", "message", "ArgumentException"), @"
Public Class [MyClass]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow indented sources not matching for VB tests, not sure why, will try to fix later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems not really depending from my code/fixer, Document editor might have issue with replacing VB node, @mavasani any suggestions?

}

[Fact]
public async Task EditorConfigConfiguredPublic_PrivateMethods_TriggeringOtherRules_DoesNotWarn()
Copy link
Contributor

@mavasani mavasani Apr 22, 2020

Choose a reason for hiding this comment

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

I think this test becomes redundant if you take the approach suggested in https://github.com/dotnet/roslyn-analyzers/pull/3500/files#r413392484, which also validates that flipping the accessibility of the method from private to public on the same test generates diagnostics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not really testing that, the MatchesConfiguredVisibility(...) method takes DiagnosticDescriptor argument, for which i passed one of the 3 descriptors of the analyzer, i was wondering if i need ot call that method for each descriptor or it will work for all, and it is testing if the api surface option works for other descriptors even if i didn't checked that especially for those descriptors

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I think it should be good to sign off once you add the suggested tests and some other minor feedback.

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.

Pass correct parameterName to ArgumentException
4 participants