-
Notifications
You must be signed in to change notification settings - Fork 975
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
Update template to use TFM-based sources; enabled latest lang features #5609
Conversation
@Pilchie seeking approval to take into RC2. @DamianEdwards this essentially wraps our templating story. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need @DamianEdwards' signoff here.
...ProjectTemplates/content/WinFormsApplication-CSharp/Company.WinFormsApplication1.csproj.user
Outdated
Show resolved
Hide resolved
<TargetFramework Condition="'$(TargetFrameworkOverride)' == ''">FrameworkParameter-windows</TargetFramework> | ||
<TargetFramework Condition="'$(TargetFrameworkOverride)' != ''">TargetFrameworkOverride-windows</TargetFramework> | ||
<RootNamespace Condition="'$(name)' != '$(name{-VALUE-FORMS-}safe_namespace)'">Company.WinFormsApplication1</RootNamespace> | ||
<LangVersion Condition="'$(langVersion)' != ''">$(ProjectLanguageVersion)</LangVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want this to show up in the project file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, has a lower case l
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a command line argument and aligns with this:
Lines 52 to 59 in 07f6de3
"langVersion": { | |
"type": "parameter", | |
"datatype": "text", | |
"description": "Sets langVersion in the created project file", | |
"defaultValue": "", | |
"replaces": "$(ProjectLanguageVersion)", | |
"displayName": "Language Version" | |
}, |
The command line args are case sensitive:
There's an open tracking issue for this dotnet/templating#3237
|
||
#endif | ||
#if (csharpFeature_FileScopedNamespaces) | ||
namespace Company.WinFormsApplication1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the decision was not to use these in the templates, and have the VS option apply formatting to decide which form to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For templates that aren't splitting their identity based on TFM and accept a language version parameter (like these), yes, I believe that was the decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes were suggested to us by the dotnet/templating team and aligns with changes made in that repo (e.g. dotnet/templating#3454).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pilchie @KathleenDollard was it that item templates would keep using block-scoped namespace declarations, but the project templates that had an option to change the language version would selectively apply file-scoped when the version was high-enough (or not specified) perhaps?
(cherry picked from commit ee658c4) Fixes dotnet#5074
b62229f
to
38a261c
Compare
@davidwengier - do you know what the conclusion was for templates and file scoped namespaces? |
In current dogfood builds, Roslyn will reformat namespaces (and other things) for any new file that is added, based on a users preferences. |
So any further objections? |
@RussKie no I think you're good to go. |
Fixes dotnet#5706 The file was lost in dotnet#5609.
(cherry picked from commit ee658c4 / #5547)
Fixes #5074
Proposed changes
Regression?
Risk
Test methodology
Manually create various apps and verify the expected content
Microsoft Reviewers: Open in CodeFlow