feat: Add ExcludeByAttribute option (#232) #233
Conversation
…menter not fail when null is sent in instead of an empty array
Codecov Report
@@ Coverage Diff @@
## master #233 +/- ##
==========================================
+ Coverage 94.91% 95.29% +0.37%
==========================================
Files 15 16 +1
Lines 1633 1699 +66
==========================================
+ Hits 1550 1619 +69
+ Misses 83 80 -3
Continue to review full report at Codecov.
|
| "ExcludeFromCoverage", | ||
| nameof(ExcludeFromCodeCoverageAttribute), | ||
| "ExcludeFromCodeCoverage" | ||
| nameof(ExcludeFromCodeCoverageAttribute) |
There was a problem hiding this comment.
This is a backward incompatible change. We should make specifying the Attribute suffix optional
There was a problem hiding this comment.
Is this actually incompatible? From what I saw of the usage even if you use the short name attribute like [ExcludeFromCoverage] the attribute name still comes across with the full name ExcludeFromCoverageAttribute. Were you trying to account for people writing custom attributes to handle excluding from coverage that were named the same as your attributes?
There was a problem hiding this comment.
It's more of a user experience thing. Since the compiler doesn't enforce the Attribute suffix I don't want us to as well. I want the user to be able to specify Obsolete and still be fine. You can simply append Attribute to the name if it's not specified
|
|
||
| ```bash | ||
| dotnet test /p:CollectCoverage=true /p:ExcludeByAttribute="ObsoleteAttribute,GeneratedCodeAttribute,CompilerGeneratedAttribute" | ||
| ``` |
There was a problem hiding this comment.
You should mention that the Attribute suffix is optional. See my other comment
There was a problem hiding this comment.
I had a sight fear since it wasn't using the fully qualified name that it could cause issues, but I can make the it try to do the short name resolution like the compiler as well.
|
I think I addressed the items in your comments @tonerdo |
amweiss
changed the title
amweiss
changed the title
amweiss
changed the title
|
@amweiss This is ready to go, modulo one more comment |
|
@tonerdo Change made. |
|
This is ready to go. Will merge once I'm ready to make a new release |
I think this is generally working, but I'm looking for a better way to automate the test for the MSBUILD task and CLI tool to make sure I didn't miss something.
If someone else can pull this and test it for their usage, I'll feel better about it and remove the [WIP] and say it's ready to merge.
Otherwise, there's also a whole bunch of random style and whitespace changes I'd like to make but don't want to mess up this PR for that and will do it in a later one. I tried to keep everything in the same pattern as the existing things, but it's not exactly how I would normally do it.