Add Namespace import based on TargetFramework and framework version and adjust VB defaults #1188
Conversation
| @@ -17,15 +17,18 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| <PropertyGroup> | |||
| <DefineConstants>$(DefineConstants),$(ImplicitFrameworkDefine)</DefineConstants> | |||
| </PropertyGroup> | |||
| <ItemGroup> | |||
| <ItemGroup Condition=" '$(DisableImplicitFrameworkReferences)' != 'true' and '$(TargetFrameworkIdentifier)' == '.NETFramework'"> | |||
There was a problem hiding this comment.
The condition on TFM doesn't sound right. So netstandard libraries don't get these default imports?
There was a problem hiding this comment.
For that reason you probably also want a different property to disable the default imports.
| @@ -17,15 +17,18 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| <PropertyGroup> | |||
| <DefineConstants>$(DefineConstants),$(ImplicitFrameworkDefine)</DefineConstants> | |||
| </PropertyGroup> | |||
| <ItemGroup> | |||
| <ItemGroup Condition=" '$(DisableImplicitFrameworkReferences)' != 'true' and '$(TargetFrameworkIdentifier)' == '.NETFramework'"> | |||
| <!-- These namespaces are present in 2.0 Framework assemblies --> | |||
| <Import Include="Microsoft.VisualBasic" /> | |||
| <Import Include="System" /> | |||
| <Import Include="System.Collections" /> | |||
| <Import Include="System.Collections.Generic" /> | |||
| <Import Include="System.Data" /> | |||
There was a problem hiding this comment.
I'd say just drop System.Data altogether since that's not in the default set of referneces for netstandard\netcore
| @@ -31,4 +31,14 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| <!-- This namespace is introduced in 4.0 Framework assemblies --> | |||
| <Import Include="System.Threading.Tasks" Condition=" '$(_TargetFrameworkVersionWithoutV)' >= '4.0' "/> | |||
| </ItemGroup> | |||
| <ItemGroup Condition=" '$(DisableImplicitFrameworkReferences)' != 'true' and '$(_IsNETCoreOrNETStandard)' == 'true'"> | |||
There was a problem hiding this comment.
We should use some new property instead of DisableImplicitFrameworkReferences - probably something like DisableImplicitImports. However for .NETFramework, we should check both properties.
|
@MattGertz for approval. |
| <OptionCompare Condition=" '$(OptionCompare)' == '' ">Binary</OptionCompare> | ||
| <OptionStrict Condition=" '$(OptionStrict)' == '' ">Off</OptionStrict> | ||
| <OptionInfer Condition=" '$(OptionInfer)' == '' ">On</OptionInfer> | ||
| <DisableImplicitNamespaceImports Condition="'$(DisableImplicitNamespaceImports)'==''">$(DisableImplicitFrameworkReferences)</DisableImplicitNamespaceImports> |
There was a problem hiding this comment.
This line (setting DisableImplicitNamespaceImports) should probably go in the .targets instead of the .props. People will set DisableImplicitFrameworkReferences in the body of their project, but if this line is here then that won't affect the default value of DisableImplicitNamespaceImports.
|
@livarcocc Is the branching plan confirmed for 15.2? Will there be a 1.1.0 branch for this PR to retarget? |
|
@basoundr yes. there will be one. I will create it tomorrow and will let you know. |
…andard projects The conditions are added to make them work appropriately Also the VB default properties are set, only if they have not been set already.
|
@livarcocc I think there is a bunch of failing of testcases(8, I think. They could be flaky as well. Not sure.) in this branch. Those failures are unrelated to my change. Could you please have someone look into it? |
basoundr
changed the title
|
@dotnet-bot retest this please |
…231.1 (dotnet#1188) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19631.1
Tagging @srivatsn @davkean @dotnet/project-system for review