Skip to content

Conversation

@ViktorHofer
Copy link
Member

Additionally format code.

cc @stephentoub

@stephentoub
Copy link
Member

What is the actual fix here? It's hard to see what's meaningful vs all of the formatting changes.

Replace the usage of TargetGroup which was corefx specific with
TargetFramework and TargetFrameworkIdentifier. Those are default SDK
properties so everyone should be able to use that package. This was
broken recently by the removal of the TargetGroup property in
dotnet/runtime.
@ViktorHofer ViktorHofer force-pushed the ViktorHofer-codeanalysis branch from bc64f8b to 9bc4159 Compare March 6, 2020 15:43
@ViktorHofer
Copy link
Member Author

ViktorHofer commented Mar 6, 2020

Sorry for the inconvenience. I was in the middle of fixing the commits when you commented. Please take another look. Commit 2 is the bugfix.


<!-- Disable any analyzers that should not run -->
<DisabledAnalyzers Condition="'$(IsTestProject)' == 'true' or '$(TargetsUnix)' == 'true' or '$(EnablePInvokeAnalyzer)' != 'true'" Include="PInvokeAnalyzer"/>
<DisabledAnalyzers Condition="'$(TargetsWindows)' == 'true' and ($(TargetGroup.Contains('net46')) or $(TargetGroup.Contains('net45')))" Include="PInvokeAnalyzer" />
Copy link
Member

Choose a reason for hiding this comment

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

Have we searched the rest of arcade for places that might still be erroneously using TargetGroup?

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed @Anipik did that when he removed the property. We have one more hit (

<_apiCompatTargetSuffix>$(TargetGroup)</_apiCompatTargetSuffix>
) which is unproblematic as it falls back to TargetFramework. We should clean that up in a subsequent PR. Not going to touch this PR as it's currently blocking my work in runtime.

@ViktorHofer ViktorHofer merged commit 6abdbe6 into master Mar 6, 2020
@ViktorHofer ViktorHofer deleted the ViktorHofer-codeanalysis branch March 6, 2020 16:58
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.

4 participants