Skip to content
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

Open
mhutch opened this issue Jul 26, 2016 · 17 comments
Open

Simplified Property Groups #820

mhutch opened this issue Jul 26, 2016 · 17 comments
Labels
Area: Language Issues impacting the MSBuild programming language. Feature - Project File Cleanup triaged
Milestone

Comments

@mhutch
Copy link

mhutch commented Jul 26, 2016

It would be nice to support a terser and more readable syntax for PropertyGroup.

By making Properties an alias for PropertyGroup and allowing properties to be provided as attributes instead / in addition to child elements, this:

<PropertyGroup>
    <Foo>A</Foo>
    <Bar>B</Bar>
</PropertyGroup>

could be simplified to

<Properties
    Foo="A"
    Bar="B"
/>

This is a pretty big readability improvement.

It would also be nice to alias Items to ItemGroup.

@NickCraver
Copy link
Member

I'd pitch a few more aliases here, for example <References> and <Includes>. In line with this issue: though they'd have no net effect, they'd make the human editable form much more readable.

Unrelated to this but along the same vein, I think something like <References Framework='net45'> would also increase readability. How much of a work tradeoff are we looking at for aliasing vs. elements with additional attribute support?

/cc @davkean

As an example, here's a file today vs. with proposed changes (I've inlined the Version attribute for comparison on both as this is already planned):

<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>

@davkean
Copy link
Member

davkean commented Nov 21, 2016

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>

@NickCraver
Copy link
Member

@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?

@davkean
Copy link
Member

davkean commented Nov 21, 2016

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"; <ItemGroup>, <PropertyGroup>.

@gulbanana
Copy link

that option looks like a large readability improvement, even if only on groups

@dasMulli
Copy link
Contributor

An additional readability improvement would be to to alias ItemGroup to Items and PropertyGroup to Properties. I'm guessing the name originated from a code element that was named to represent a collection..

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
Copy link
Member

@dasMulli I'm confused...that was the original issue. Are you talking about other changes, or just adding <Items> to the list?

@mhutch Perhaps an update up top (if you agree with these) with an overall alias list would be helpful?

@dasMulli
Copy link
Contributor

@NickCraver slightly different: The original proposal of the issue is adding the properties as attributes, which is a more substantial change than "just" aliasing.

@mhutch
Copy link
Author

mhutch commented Nov 21, 2016

Added the Items alias.

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.

@mhutch
Copy link
Author

mhutch commented Nov 21, 2016

Also, check out #821

@rainersigwald
Copy link
Member

My thoughts on PropertyGroup to Properties (and ItemGroup to Items) aliasing: I wish that had been the original name, but I don't see enough benefit from enabling it now. We'd have to allow the old names and the new names, making parsing and persisting projects after OM manipulation more complicated both for us and for the folks that treat MSBuild projects as straight XML (that I wish didn't happen but does).

@mhutch
Copy link
Author

mhutch commented Nov 21, 2016

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 :)

@AlgorithmsAreCool
Copy link

@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.

@davidfowl
Copy link
Member

@rainersigwald how much pizza and beer do you need? We can start a gofund me.

@rainersigwald
Copy link
Member

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.

@AlgorithmsAreCool
Copy link

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 <project>, <ItemGroup>, <PropertyGroup> and the new way uses <*NewProject*>, <Includes>, <References>, ...

Where <*NewProject*> is something other than <Project> to make mode detection easy.

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...

@aL3891
Copy link

aL3891 commented Dec 14, 2016

I made an issue kind of similar to this but with a slightly different take, linking it just to keep track
#1453

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Language Issues impacting the MSBuild programming language. Feature - Project File Cleanup triaged
Projects
None yet
Development

No branches or pull requests