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

Update C# templates to introduce file scoped namespaces #4911

Merged
merged 8 commits into from
Dec 17, 2024

Conversation

haonanttt
Copy link
Contributor

Purpose

Regarding to #3329, creating this PR to introduce file scoped namespaces.

What have changed

For all .cs files inside dev/VSIX/ItemTemplates and dev/VSIX/ProjectTemplates, the template code format have been changed to file scoped namespaces:

namespace MyNamespace;

public sealed partial class MainWindow : Window
{
    public MainWindow()
    {
        ...
    }
}

@haonanttt
Copy link
Contributor Author

1 more thing to be confirmed:
Should the .cs files inside dev/VSIX/Extension and dev/VSIX/Shared also be changed?

@michael-hawker
Copy link

1 more thing to be confirmed: Should the .cs files inside dev/VSIX/Extension and dev/VSIX/Shared also be changed?

I'm not as familiar with those, but it doesn't hurt to apply this change to every C# file across the entire repo.

File-scoped namespaces were added for .NET 6, which should be everything here.

Copy link

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦙❤️ Awesome, thanks @haonanttt for putting this together! 🎉🎉🎉 Couple of suggestions to improve partial class coverage for AOT and source generators.

I thought I remembered another small thing related to templates, but can't find it in my chat history. Nothing came to mind when going through the review. @niels9001 want to do a quick pass to see if I missed anything?

1. mark all classes 'partial'
2. align the MainWindow to be an empty Grid without the Button and handler
@haonanttt haonanttt marked this pull request as ready for review November 22, 2024 09:23
@niels9001
Copy link

@haonanttt Love these changes 🚀! Since we are removing the Button click event, we should remove it from the Cpp templates as well?

With .NET9 WPF now comes with modern theming and has Mica enabled as default maybe this is something we should enable as well? Especially since this is something that is a default design pattern across Windows, and are just a 3 lines of XAML.

@codendone codendone requested a review from Scottj1s November 22, 2024 18:43
@michael-hawker
Copy link

@niels9001 good call out! This may have been what I was thinking about unconsciously. It was pretty jarring to see the difference with the new WPF template/theming enabled and the WinUI window just being a black box during our .NET Conf demo:

image

@haonanttt
Copy link
Contributor Author

@haonanttt please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"

@haonanttt
Copy link
Contributor Author

Current changes:

  1. introduce file scoped namespaces for .cs files inside dev/VSIX
  2. Make sure all .cs files using partial classes
  3. Enable Mica for all window-level files

Need discussion:

  1. Shall we still keep the button and handler or make everything empty grid

2. Remove Background="{ThemeResource ApplicationPageBackgroundThemeBrush}" from Blank Page item template
Copy link

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Just did a quick pass to apply some C# style guidelines as suggestions which can be applied in batch. Mostly to remove this. and the m_ to just _ for window. (I linked to references in those first comments.) The only thing I couldn't make as a suggest was moving the _window member to the top of the class.

Thanks so much for going through this with us, this is going to be a great improvement to the templates to make it more streamlined for devs to get work done! 🦙❤️

dev/VSIX/Shared/WizardImplementation.cs Outdated Show resolved Hide resolved
dev/VSIX/Shared/WizardImplementation.cs Outdated Show resolved Hide resolved
dev/VSIX/Shared/WizardImplementation.cs Outdated Show resolved Hide resolved
dev/VSIX/Shared/WizardImplementation.cs Outdated Show resolved Hide resolved
dev/VSIX/Shared/WizardImplementation.cs Outdated Show resolved Hide resolved
2. Add empty line between functions
@haonanttt haonanttt self-assigned this Nov 28, 2024
@michael-hawker michael-hawker dismissed niels9001’s stale review December 4, 2024 07:21

Stale, feedback addressed

@haonanttt
Copy link
Contributor Author

Current status:

  • Comments from Michael and Niels have been resolved.

Next steps:

  • Will move on the PR after Scott's review

@michael-hawker
Copy link

michael-hawker commented Dec 6, 2024

FYI @Scottj1s, Niels and I are all good here as external approvers; we really appreciate the work @haonanttt has done with us here to modernize the templates and bring them up-to-date with all the latest .NET features/patterns. 🦙❤️

I believe we addressed @codendone's initial concern about the Button in the comment thread, but happy to chat about any concerns you have, feel free to ping me if you have any - otherwise hopefully we can just merge. Thanks!

@Scottj1s
Copy link
Member

Scottj1s commented Dec 6, 2024

@haonanttt I'll add my thanks too, as well as to @mrlacey, @dotMorten, who were involved in an earlier discussion on this, and to @wjk, who authored a similar PR a few years ago, later closed. I appreciate the need for discussion, but we clearly never got around to one, and the frustrations remain. My preference is to bias for action here and course correct if necessary.

A few things to consider:

  • There is much history here, which I myself don't recall. The C# templates were inspired by the C++, in turn cribbed from C++/WinRT. I have no problem with divergence from the C++/WinRT templates, but we should at least be consistent between C++ and C# templates here - removing the Button in both (see William's PR).
  • Ideally, we would also remove the unused C++ project template MyProperty (see William's PR) - however, there may be a technical limitation that requires at least one member in the class IDL.
  • Since we're removing the Button, we could consider adding a readme.txt, as we have for the C++ templates (e.g., here), to include a link to the Windows App SDK Samples, and/or add a comment in the Xaml/C# with that link.

@Scottj1s
Copy link
Member

Scottj1s commented Dec 6, 2024

... we should at least be consistent between C++ and C# templates here - removing the Button in both (see William's PR).

Ah, wasn't paying close attention - I see we've removed Button across the board

@michael-hawker
Copy link

@Scottj1s I like the idea of a readme, but I know for UWP VS would just pop open the 'getting started' guide when you started a new project to get these resources:

image

They do have this comment in the MainPage.xaml.cs:

// The Blank Page item template is documented at https://go.microsoft.com/fwlink/?LinkId=402352&clcid=0x409


For WinUI 3 by comparison, that 'getting started' guide does not appear - are we missing a flag or something - or was this just never ported over in VS?

image

And we already have this comment in the MainWindow.xaml.cs file:

// To learn more about WinUI, the WinUI project structure,
// and more about our project templates, see: http://aka.ms/winui-project-info.

But this landing page is a bit sparse, should we use a different aka.ms link here or improve that doc page? FYI @niels9001

@michael-hawker
Copy link

@Scottj1s could we merge these technical improvements into the templates now? And then take a follow-up with a task to talk about improving the getting started pages in VS across the board? It seems that may be unique to UWP, but a great feature. Like WPF doesn't do it either and just throws you into MainWindow.xaml:

image

Our other templates don't add readmes (as then folks tend to never go update those so we could get repos people push to GitHub that just have that file everywhere muddling search results). It really seems to be supplanting this feature that seems supported by VS somehow/somewhere for at least some project types, so it'd be great if we could work to improve this everywhere for everyone for all project types working across the VS ecosystem.

And in the interim, we should update the existing link's comment or improve the page it points to in the docs. @niels9001 is that a good aka.ms link which of those two comment options should we take?

Copy link
Contributor

@dotMorten dotMorten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh YES! This is such a nice cleanup of the starter template! Ship it!

Also regarding the readme discussion. Please don't add junk to the project structure. This might be better as a Visual Studio feature, with the little yellow tips bar that is sometimes on top of the file tabs (I've seen MAUI often use this)

@michael-hawker
Copy link

Leaving this here for the future, @niels9001 found the introduction to start pages in VS blog here: https://devblogs.microsoft.com/visualstudio/walkthrough-creating-a-custom-start-page-part-1/

Seems like we should create follow-on tasks for both WPF and WinUI here (for both C++ and C#) to work to adding these into the templates.

@RyanLua
Copy link

RyanLua commented Dec 7, 2024

I haven't seen this mentioned here but the default .NET .editorconfig file defines block scoped namespace so Visual Studio or other tools using .editorconfig may format using block scoped.

csharp_style_namespace_declarations = block_scoped

Just letting my thoughts since I don't see this being addressed. Nothing that I think this PR can do since this is more of a Visual Studio problem than WASDK.

@ghost1372
Copy link
Contributor

ghost1372 commented Dec 7, 2024

Hi @michael-hawker
The link you provided is an old article and the links and templates are not available.
After a few hours of struggling, I was able to load a user control into the Tool Window.😁

image

@ghost1372
Copy link
Contributor

I haven't seen this mentioned here but the default .NET .editorconfig file defines block scoped namespace so Visual Studio or other tools using .editorconfig may format using block scoped.

csharp_style_namespace_declarations = block_scoped

Just letting my thoughts since I don't see this being addressed. Nothing that I think this PR can do since this is more of a Visual Studio problem than WASDK.

Absolutely correct
I have faced this problem before and had to use the .editorconfig file for my VSIX.
file scoped namespaces should be fixed by Visual Studio. The only solution is to add the .editorconfig file via VSIX.

@michael-hawker
Copy link

@haonanttt @Scottj1s now that we have this approved, what's left before we merge? How do these get integrated into releases from the repo here? Will it just be in the next latest drop on NuGet that pulls these in?

@haonanttt
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@michael-hawker
Copy link

Looks like some of the auxiliary project types target older .NET runtimes or something?

dev\VSIX\Extension\Cs\Dev17\VSPackage.cs(10,1): Error CS8370: Feature 'file-scoped namespace' is not available in C# 7.3. Please use language version 10.0 or greater.

dev\VSIX\Extension\Cs\Dev17\VSPackage.cs

If this is just for the package itself, we should be fine to just add <LangVersion> to the csproj here? Otherwise, we can put it back for this one, it matters less if it's not consumed by external folks and just a helper class (I'm not familiar with the specific of the VSIX config here beyond the C# templates).

dev\VSIX\Shared\WizardImplementation.cs

The same csproj above manages this file as well, though there's also an include of it in the C++ project as well?

<Compile Include="..\..\..\Shared\WizardImplementation.cs" Link="WizardImplementation.cs" />

Not as sure what this even means here. If these are just internal files to the VSIX, we don't have to update the scope here, or we can try adding <LangVersion> to both projects and see if that resolves it.

@haonanttt
Copy link
Contributor Author

Investigated and informed the file dev\VSIX\Extension\Cs\Dev17\VSPackage.cs and dev\VSIX\Shared\WizardImplementation.cs is used for project setup, will not show as some templates. So decide to revert the file scoped namespace for them to get less impact for those files.

@haonanttt
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@haonanttt haonanttt merged commit a0a435d into main Dec 17, 2024
26 checks passed
@haonanttt haonanttt deleted the user/haonantang/FileScopedNamespaces branch December 17, 2024 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants