-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Simplified Property Groups #820
Comments
I'd pitch a few more aliases here, for example Unrelated to this but along the same vein, I think something like /cc @davkean As an example, here's a file today vs. with proposed changes (I've inlined the <Project ToolsVersion="15.0">
<Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" />
<PropertyGroup>
<TargetFrameworks>net45;netstandard1.5</TargetFrameworks>
<AssemblyName>MiniProfiler.Shared</AssemblyName>
<OutputType>Library</OutputType>
<OutputPath>bin\</OutputPath>
<SignAssembly>true</SignAssembly>
<AssemblyOriginatorKeyFile>..\MiniProfiler\miniprofiler.snk</AssemblyOriginatorKeyFile>
</PropertyGroup>
<ItemGroup>
<Compile Include="**\*.cs" />
<EmbeddedResource Include="**\*.resx" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Microsoft.NET.Sdk" Version="1.0.0-alpha-20161104-2" PrivateAssets="All"/>
<PackageReference Include="NETStandard.Library" Version="1.6.1"/>
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.5'">
<PackageReference Include="System.ComponentModel.Primitives" Version="4.3.0"/>
<PackageReference Include="System.Data.Common" Version="4.3.0"/>
<PackageReference Include="System.Diagnostics.StackTrace" Version="4.3.0"/>
<PackageReference Include="System.Dynamic.Runtime" Version="4.3.0"/>
<PackageReference Include="System.Reflection.Emit.Lightweight" Version="4.3.0"/>
<PackageReference Include="System.Runtime.Extensions" Version="4.3.0"/>
<PackageReference Include="System.Runtime.Serialization.Primitives" Version="4.3.0"/>
<PackageReference Include="System.Threading.Tasks.Parallel" Version="4.3.0"/>
<PackageReference Include="Newtonsoft.Json" Version="9.0.1"/>
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'net45'">
<Reference Include="Microsoft.CSharp" />
<Reference Include="System" />
<Reference Include="System.Configuration" />
<Reference Include="System.Data" />
<Reference Include="System.Data.Linq" />
<Reference Include="System.Runtime.Serialization" />
<Reference Include="System.Transactions" />
<Reference Include="System.Web" />
<Reference Include="System.Web.Extensions" />
<Reference Include="System.Xml" />
</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
</Project> Proposed: <Project ToolsVersion="15.0">
<Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" />
<Properties>
<TargetFrameworks>net45;netstandard1.5</TargetFrameworks>
<AssemblyName>MiniProfiler.Shared</AssemblyName>
<OutputType>Library</OutputType>
<OutputPath>bin\</OutputPath>
<SignAssembly>true</SignAssembly>
<AssemblyOriginatorKeyFile>..\MiniProfiler\miniprofiler.snk</AssemblyOriginatorKeyFile>
</Properties>
<Includes>
<Compile Include="**\*.cs" />
<EmbeddedResource Include="**\*.resx" />
</Includes>
<References>
<PackageReference Include="Microsoft.NET.Sdk" Version="1.0.0-alpha-20161104-2" PrivateAssets="All"/>
<PackageReference Include="NETStandard.Library" Version="1.6.1"/>
</References>
<References Framework="netstandard1.5">
<PackageReference Include="System.ComponentModel.Primitives" Version="4.3.0"/>
<PackageReference Include="System.Data.Common" Version="4.3.0"/>
<PackageReference Include="System.Diagnostics.StackTrace" Version="4.3.0"/>
<PackageReference Include="System.Dynamic.Runtime" Version="4.3.0"/>
<PackageReference Include="System.Reflection.Emit.Lightweight" Version="4.3.0"/>
<PackageReference Include="System.Runtime.Extensions" Version="4.3.0"/>
<PackageReference Include="System.Runtime.Serialization.Primitives" Version="4.3.0"/>
<PackageReference Include="System.Threading.Tasks.Parallel" Version="4.3.0"/>
<PackageReference Include="Newtonsoft.Json" Version="9.0.1"/>
</References>
<References Framework="net45">
<Reference Include="Microsoft.CSharp" />
<Reference Include="System" />
<Reference Include="System.Configuration" />
<Reference Include="System.Data" />
<Reference Include="System.Data.Linq" />
<Reference Include="System.Runtime.Serialization" />
<Reference Include="System.Transactions" />
<Reference Include="System.Web" />
<Reference Include="System.Web.Extensions" />
<Reference Include="System.Xml" />
</References>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
</Project> |
One thought I had was that instead of MSBuild having special knowledge of individual properties, such as "Framework" (which it doesn't do today), it would instead simply allow a simpler condition syntax. Therefore something like this: <ItemGroup TargetFramework="net45">
...
</ItemGroup> Would be treated as if I wrote: <ItemGroup Condition="'$(TargetFramework)'=='net45">
<ItemGroup> Then you could do something like: <PropertyGroup TargetFramework="net45" Configuration="Debug">
<!-- Only applies when targeting .NET 4.5 under Debug -->
</PropertyGroup> Or: <ItemGroup>
<!-- Only reference when targeting UWP -->
<ProjectReference Include="UWPSupport.csproj" TargetFramework="UWP" />
</ItemGroup> |
@davkean That's an extremely attractive option - what's the thinking on existing attributes and collision with property names with this layout? Known attributes would be preferred? I could see that, but then if you didn't know it was an attribute would be fairly confusing. I wonder if we could figure out a way to indicate it's a conditional and bypass the conflict case. Or am I over-analyzing this? |
Not over analyzing this, there is a concern. Especially that item metadata has been prompted to attributes -even more likely that there will be clashes. I'll let the MSBuild chime into the issues - maybe you could make it less likely by only allowing the syntax on "groups"; |
that option looks like a large readability improvement, even if only on groups |
An additional readability improvement would be to to alias For readability, this would be a small change but a huge improvement: <Properties>
<Awesome>true</Awesome>
</Properties>
<Items>
<Compile Include="..." />
</Items> If i understand msbuild correctly, there could be an issue if there was a task named like that when adding properties or items inside of a task. |
@NickCraver slightly different: The original proposal of the issue is adding the properties as attributes, which is a more substantial change than "just" aliasing. |
Added the I think the simplified conditions could work on itemgroups, properties, targets, tasks etc. But on propertygroups it would directly collide with my proposal, and on items there would be a very high risk of collision with metadata attributes. So... I'm not sure what I think about it. |
Also, check out #821 |
My thoughts on |
I personally think anything uncontentious we can do to make MSBuild more readable is worth doing, but I'm not the one who has to implement it :) |
@rainersigwald I would venture that it is worth it to get the syntax as clean and friendly as possible to help make the platform as easy to hop into as possible. Lets not forget the bad foot that this repo started on. Msbuild has some baggage but has made some great progress. Be bold, we have minds to win. If that argument doesn't work then I submit the following: Please. I'll buy pizza. |
@rainersigwald how much pizza and beer do you need? We can start a gofund me. |
I don't need bribes, I need someone to tell me how the Construction APIs will deal with this situation without making my head explode. It seems like a lot of work that's better spent doing something like building IntelliSense so you don't have to know the right magic keyword and are just guided onto the right path. |
Spitballing an idea here, Am I correct that the newer syntax is just sugar? Or is there novel functionality being added? (I think globbing was added for example) If the core functionality is the same then what about just having two separate project schemas? So the old way works with Where The new syntax could have all the handy defaults, assumptions and friendly names that made project.json simple. Since this is all standard XML then a sufficiently smart XSLT post-parse should make this 'trivial' to desugar (similar to compiler 'lowering') to the traditional syntax. But I have a feeling I am underestimating the reverse compat requirements here however... |
I made an issue kind of similar to this but with a slightly different take, linking it just to keep track |
It would be nice to support a terser and more readable syntax for
PropertyGroup
.By making
Properties
an alias forPropertyGroup
and allowing properties to be provided as attributes instead / in addition to child elements, this:could be simplified to
This is a pretty big readability improvement.
It would also be nice to alias
Items
toItemGroup
.The text was updated successfully, but these errors were encountered: