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

[Suggestion] Getting rid of PropertyGroup/ItemGroup #1236

Closed
MaximRouiller opened this issue Oct 24, 2016 · 21 comments
Closed

[Suggestion] Getting rid of PropertyGroup/ItemGroup #1236

MaximRouiller opened this issue Oct 24, 2016 · 21 comments
Labels

Comments

@MaximRouiller
Copy link

Copy from: dotnet/project-system#632
As requested by: @srivatsn
Originally by: @filipw

How about a csproj without <PropertyGroup>, <ItemGroup>, the 2 default imports and an implicit <Compile Include="**\*.cs" />

<Project>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp1.0</TargetFramework>

   <PackageReference Include="Microsoft.NETCore.App" Version="1.0.0" />
   <PackageReference Include="Microsoft.NET.SDK" Version="1.0.0" />
</Project>
@MaximRouiller
Copy link
Author

Original discussion thread in dotnet/project-system#628

@dasMulli
Copy link
Contributor

Not sure if it is worth a separate issue, but could PropertyGroup and ItemGroup be aliased to Properties and Items? It would probably break if a task named like that exists (any known usage?) but much nicer to read. ("Group" is just stating the obvious and shifts focus away from its "here's some properties" purpose. If there actually is something special about a specific group, there'd be a condition anyway...).

@Mike-E-angelo
Copy link

Mike-E-angelo commented Oct 24, 2016

Actually @dasMulli it is a separate issue in #820 if I am not mistaken. :)

@danmoseley
Copy link
Member

It's a good idea (to optionally skip both) which would make the file cleaner and I don't think it's breaking. The only reason I can recall for having them was (1) you need them if you want a condition and (2) it helps intellisense in the file -- which was driven from the XSD so it was never very good -- eg it can prompt for Include when it's inside an item group.

@Mike-E-angelo
Copy link

Geeze, two downvotes on this issue, which is meant to help clean up the decade-plus-old format and make it more relevant and palatable for the current developer environment/ecosystem.

Tough crowd... 😛

@gulbanana
Copy link

alas, outcome defies intention

@miloush
Copy link

miloush commented Oct 29, 2016

@Mike-EEE That is not a proposal, make it clearer what you are suggesting and address questions that have been raised.

  1. Do you want to make them optional or remove them?
  2. Does your proposal include ItemDefintionGroup?
  3. How would the system know the item is a property or item or item definition?
  4. What do you propose it would happen if property and item have the same name?
  5. How would you condition group of properties/items/item definitions?
  6. Plan for the IntelliSense mess that your proposal brings?
  7. How about tools that validate the files against XML schemas? Two schemas?
  8. What does your proposal bring, or in your words, how is it more relevant and more palatable for the current environment?

@Mike-E-angelo
Copy link

Mike-E-angelo commented Oct 29, 2016

@miloush ... if that was the problem that led to your downvote, why not be a champ and post these questions when @MaximRouiller originally posted the issue? That seems like a much more constructive way to help build the community here, ala StackOverflow.

And also, I believe I am allowed to provide commentary/observations without providing solutions. If this is in violation of a rule in your repo, please kindly point me to it. Regardless (and TBH), I think this whole path/issue is a fool's errand and what we really need to be doing is providing a POCO-based model to truly bring MSBuild back into relevancy, as well as positive net perception and therefore adoption. So, I feel I have already answered your questions (and so much more) in my suggested route, which just so happens to be the most upvoted issue in this repo. But feel free to downvote it as well, since that seems to be your thing. 😉 ❤️

I must say that I find it truly fascinating the amount of questions that are proposed in opposition to issues that are meant to improve this product, when MSFT engineers have historically tackled and solved much, much, MUCH more difficult problems. There seems to be a lot of cantitude (can't-do-it-attitude) in this repo, where more energy will be placed into finding reasons not to do something rather than find ways to simply improve the product -- like I experience in other repos. It's concerning.

Not that I really have room to complain as I haven't lifted a finger for one line of code. 😄 Not that I have really felt welcomed to do this, either.

As @gulbanana smartly, deftly, and aptly states, outcome defies intention, where we are all seem content to simply sit on our hands, caked in ~15 year-old technology.

@miloush
Copy link

miloush commented Oct 29, 2016

@Mike-EEE No, I have downvoted because I disagree with the idea.

The rest was an attempt to respect your opinion and help you come up with an actual proposal nevertheless.

If engineers have tackled and solved much more difficult problems, it is because they were asking and trying to answer similar questions themselves.

@Mike-E-angelo
Copy link

Mike-E-angelo commented Oct 29, 2016

Fair enough, @miloush. I obviously didn't feel that way, based on previous experiences in this repo, both personally and witnessed. It is easy to misinterpret the tone as such. I do appreciate the gesture, however, I believe/feel it's better for someone such as @MaximRouiller to dive into your questions.

From my perspective and understanding, I will say that the objective is to keep the MSBuild schema in tact as it rests now, but being able to define the files w/o the Compile, PropertyGroup and ItemGroup elements. That is, those elements still exist, but they are not explicitly defined in the file and have their own internally defined defaults. The result of which makes the file more terse and therefore easier to create by hand and for the eyes (and mind) to digest when read.

When those elements are explicitly defined in the file, they override the internally defined defaults as mentioned above. So, everything stays the same as it is now, but less XML is required in the file and default logic/values move into the background processing. Hope that helps. 👍

@MaximRouiller
Copy link
Author

Guys, I copy/pasted a suggestion. I don't know exactly what was behind it.

My opinion? Make them optional.

If you look at the description of the issue, you will find:

Copy from: dotnet/project-system#632
As requested by: @srivatsn
Originally by: @filipw

With the rest as quote. It's Halloween 👻 . I'm having my first coffee. Give me 10 minutes to come up with some kind of joke. 😛

@mhutch
Copy link
Member

mhutch commented Dec 14, 2016

I'm not sure whether I like this or not... but the presence of the Include/Remove/Update attributes would make it easy to tell items apart from properties.

@kzu
Copy link
Contributor

kzu commented Mar 1, 2017

I'd like to see this implemented too. There's no confusion when item require the Include/Remove/Update attributes, and properties are simple elements with no attributes at all and are the only ones that can have text content. For example, the following item is invalid:

<Content Include="readme.txt">This is great!</Content>

And the following is obviously a property since it has no attributes:

<Content>This is great!</Content>

In fact, I think just the presence of any attributes would signal that it's an item, requiring Include and interpreting the attributes as item metadata.

@rainersigwald
Copy link
Member

I'm generally against this idea, because I think it makes projects harder to understand for someone who doesn't know MSBuild deeply--which is most MSBuild users.

You must have the knowledge that you reference properties with $(PropertyName) and items with @(ItemName). That isn't really affected by this proposal.

As is, there's a clue as to what a thing is embedded in its definition:

<PropertyGroup>
  <NamedThing1>value1</NamedThing1>
</PropertyGroup>
<ItemGroup>
  <NamedThing2 Include="value2" />
</ItemGroup>

<Target Name="Example">
  <Message Text="Uses: $(NamedThing1) and @(NamedThing2)" />
</Target>

With this change, it'd be unambiguous to the parser (I think), but confusing to the reader:

<NamedThing1>value1</NamedThing1>
<NamedThing2 Include="value2" />

<Target Name="Example">
  <Message Text="Uses: $(NamedThing1) and @(NamedThing2)" />
</Target>

One major mitigation to this would be a language service/syntax highlighter (#1774) that disambiguated properties and items through color or underlining or italics or something, and helped the typist find the right syntax to use a property or item ("did you mean to reference the item @(NamedThing2) instead of the nonexistent property $(NamedThing2)?"). That might let us have the cake (really concise proj files) and eat it too (be able to understand them later).

@rainersigwald
Copy link
Member

@kzu

In fact, I think just the presence of any attributes would signal that it's an item, requiring Include and interpreting the attributes as item metadata.

Even Condition=""? That's a fun one.

@kzu
Copy link
Contributor

kzu commented Mar 1, 2017

Here's something likely confusing for someone that doesn't know C# "deeply":

public string RootDir => "C:";

vs

public string RootDir() => "C:";

We're talking two chars vs a whole word (Include) plus different content model for props vs items ;).

Also, the example would more typically be:

<NamedThing1>value1</NamedThing1>

<NamedThing2 Include="value2;value3" />
<NamedThing2 Include="value4" />
<NamedThing2 Include="value5" />

Where it's more obvious that items are many, whereas props are scalars.

Also, do we really need to optimize for someone who doesn't know the difference between $() and @()? You can't do anything at all in MSBuild if you don't know even that. It's like not knowing when to use a semi-colon in C# ;).

Items, properties, targets and tasks are the four fundamental basic concepts you can learn from reading a blog post on MSBuild. Maintaining verbosity for the benefit of those who don't grasp even these four key concepts to the detriment of the several of billions that do, seems... sad! ;)

And FWIW, I believe those that dare to crack open a .csproj have the guts to learn that much at a bare minimum.

Granted, just like the lambda example, this removal of groups could be for advanced users, just like the attribute syntax and so on. There's nothing wrong IMHO with having both. Document the verbose version for beginners, and have the advanced users leverage a more terse syntax.

@rainersigwald
Copy link
Member

Also, do we really need to optimize for someone who doesn't know the difference between $() and @()?

No, I don't think so. That's what I was trying to get at with my "you must have the knowledge" sentence.

However, I think there's a difference between "I defined something in a PropertyGroup above so I know I need to use $()" and "I know that because I didn't type Include above I need to use $()".

In terms of C# analogies: I think there's a big difference there too, and it's that basically no one chooses MSBuild as a programming language. All of our users are here for the rest of the .NET ecosystem. I don't want to imply "to be a .NET Developer, you must have a very good understanding of MSBuild as well as a good understanding of C# or VB." That's the principle that guides me to prefer the status quo over this change--you have to know items vs. properties but not maintain a mental model that helps you see the type system from minimal annotation.

(Obviously within the constraints of the language we have--as I often disclaim, I wouldn't design it this way today, but we have compatibility constraints.)

@kzu
Copy link
Contributor

kzu commented Mar 1, 2017

Fair enough. I still think for advanced users, it should be an option. Kinda like PropertyGroup/ItemGroup are the Option=Strict of MSBuild ;).

Maybe with the aliasing of PropertyGroup -> Properties and ItemGroup -> Items, it improves a bit already.

@MaximRouiller
Copy link
Author

Any progress on this? I don't think it looks relevant now with the new project system and everything.

Can I close?

@rainersigwald
Copy link
Member

I don't have any objection to closing this, so let's do that.

@danielchalmers
Copy link

do you think this could be reopened? @rainersigwald @MaximRouiller
I believe there's still interest in this even with all the improvements that have already been made.

it has 24 👍 here, 21 👍 on dotnet/project-system#632, and the original example of

<Project>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp1.0</TargetFramework>

   <PackageReference Include="Microsoft.NETCore.App" Version="1.0.0" />
   <PackageReference Include="Microsoft.NET.SDK" Version="1.0.0" />
</Project>

with Groups removed was not far off from what we have now, so I feel the suggestion still stands

@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests