#if'ed the definition of ExcludeFromCodeCoverageAttribute depending o… #2332
Conversation
|
There's still this Would the fact that this is not a real C# file bother the format tool? Maybe the templates should be excluded from the analysis? What's weird too is that |
Yep, that sounds like a good idea. I don't know |
| #define SHOULD_DEFINE_EXCLUDE_FROM_CODE_COVERAGE | ||
| #elif NETSTANDARD1_0 || NETSTANDARD1_1 || NETSTANDARD1_2 || NETSTANDARD1_3 || NETSTANDARD1_4 || NETSTANDARD1_5 || NETSTANDARD1_6 | ||
| #define SHOULD_DEFINE_EXCLUDE_FROM_CODE_COVERAGE | ||
| #endif |
There was a problem hiding this comment.
Why all these #elif's? Couldn't one big #if cover all targets?
There was a problem hiding this comment.
That's just an attempt to split the very long #if line... I did something similar in the VB file, but couldn't do it in F#.
| @@ -8,6 +8,22 @@ | |||
| // </auto-generated> | |||
| //------------------------------------------------------------------------------ | |||
|
|
|||
| #if NET20 || NET35 || NETCOREAPP1_0 || NETCOREAPP1_1 || NETSTANDARD1_0 || NETSTANDARD1_1 || NETSTANDARD1_2 || NETSTANDARD1_3 || NETSTANDARD1_4 || NETSTANDARD1_5 || NETSTANDARD1_6 | |||
There was a problem hiding this comment.
Shouldn't SHOULD_DEFINE_EXCLUDE_FROM_CODE_COVERAGE be defined in F# as well?
There was a problem hiding this comment.
There's no preprocessor instruction in F# that allows defining a constant (when you need one you have to pass it to the compiler with a -d argument). And the SHOULD_DEFINE_EXCLUDE_FROM_CODE_COVERAGE constant has no special meaning, it's just used to split the #if as I said above.
I agree this is not consistant. If you prefer, I can have everything look more like the F# version.
There was a problem hiding this comment.
I would prefer consistency. Hopefully, we can replace these #if statements with a check before the files are generated in the first place.
There was a problem hiding this comment.
Hopefully, we can replace these #if statements with a check before the files are generated in the first place.
I suppose you are referring to a solution similar to what was suggested in the issue: having the support of the attribute detected by MSBuild? That would probably work but:
- I'm not that familiar (yet) with how MSBuild code works in GitVersion,
- Will you ever want to support generation of
GitVersionInformationclass from the command-line utility? If so, I think we can't rely on constants defined in a MSBuild file that would not exist in this case.
Anyway, I don't have clear ideas to how we can reliably get rid of this ugly #if, but in the meantime and if you're ok with this. I'll simply have everything look more like the F# version.
There was a problem hiding this comment.
I suppose you are referring to a solution similar to what was suggested in the issue: having the support of the attribute detected by MSBuild?
Correct. I agree that the solution is difficult, though, which is why I think we should go ahead with this #if-based solution first.
Anyway, I don't have clear ideas to how we can reliably get rid of this ugly #if, but in the meantime and if you're ok with this. I'll simply have everything look more like the F# version.
👍
| #define SHOULD_DEFINE_EXCLUDE_FROM_CODE_COVERAGE | ||
| #elif NETSTANDARD1_0 || NETSTANDARD1_1 || NETSTANDARD1_2 || NETSTANDARD1_3 || NETSTANDARD1_4 || NETSTANDARD1_5 || NETSTANDARD1_6 | ||
| #define SHOULD_DEFINE_EXCLUDE_FROM_CODE_COVERAGE | ||
| #endif |
| @@ -8,6 +8,22 @@ | |||
| // </auto-generated> | |||
| //------------------------------------------------------------------------------ | |||
|
|
|||
| #if NET20 || NET35 || NETCOREAPP1_0 || NETCOREAPP1_1 || NETSTANDARD1_0 || NETSTANDARD1_1 || NETSTANDARD1_2 || NETSTANDARD1_3 || NETSTANDARD1_4 || NETSTANDARD1_5 || NETSTANDARD1_6 | |||
There was a problem hiding this comment.
I would prefer consistency. Hopefully, we can replace these #if statements with a check before the files are generated in the first place.
|
Brilliant, thank you so much @odalet! 🙏 |
…n the target framework
The
ExcludeFromCodeCoverageAttributeis not supported on some older .NET framework targets:In order for projects targeting some of these frameworks to be able to use GitVersion,
ExcludeFromCodeCoverageAttributeneeds to be defined conditionally when targeting these frameworks.This is exactly what this PR does.
This PR fixes #2330.
I used a side-project to ensure my changes were correct (see https://github.com/odalet/GitVersionTests). Unfortunately I wasn't able to enforce these tests with proper unit tests (I think it is not a trivial task to be able to ensure correct compilation of projects across all framework x language combinations).