search

CommunityToolkit / WindowsCommunityToolkit

The Windows Community Toolkit is a collection of helpers, extensions, and custom controls. It simplifies and demonstrates common developer tasks building .NET apps with UWP and the Windows App SDK / WinUI 3 for Windows 10 and Windows 11. The toolkit is part of the .NET Foundation.
https://docs.microsoft.com/windows/communitytoolkit/
Other
5.89k stars 1.38k forks source link

Review namespaces and folder structure coupling #3422

Closed mrlacey closed 3 years ago

mrlacey commented 4 years ago

Describe the problem this feature would solve

The way namespaces are currently used across the codebase is inconsistent.

This is a follow-on from the discussion started around #3405

Describe the solution

Ideally, there should be a standard that is defined for how namespaces are used. This should be documented. The existing code should then be updated to this standard.

I propose that a single namespace should be used by each package unless there is a need or strong reason not to, or it is necessary for compatibility with external standards (such as the attribute classes being in the System.Diagnostics.CodeAnalysis namespace. Using multiple namespaces when they are not required can lead to confusion on the part of the developer consuming them. Having to use multiple namespaces when they are not necessary also adds overhead to the consuming code (even if just in adding using directives). VS Tooling is able to infer the desired namespaces and offer suggestions to add them but they don't add any value when all they do is reflect the folder structure of the underlying source.

This may result in some breaking changes and so should be done as part of a major release.

Describe alternatives you've considered

Live with the inconsistency that currently exists.

Additional context & Screenshots

From a quick scan across the codebase, different projects/packages typically either use one namespace of everything or have lots of namespaces that reflect the folder structure.

Some noted from my quick review:

The above is based on a brief review of the current code. I'd be happy to take a fuller look at all the code if further investigation of this issue and adopting a consistent convention is agreed upon.

Note also that the HighPerformance and MVVM packages are some of the biggest users of multiple namespaces. As there are the newest packages, changes in them may be impactful. However, if changes are to be made to the MVVM package it will be better to do this as soon as possible.

ghost commented 4 years ago

Hello, 'mrlacey! Thanks for submitting a new feature request. I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future!

Sergio0694 commented 4 years ago

I'm all for fixing the inconsistencies you mentioned, like:

I agree that there shouldn't be weird situations with single items breaking the pattern used in that package, or being the only exception compared to all other types in that folder or in that parent namespace, that sounds great! 👍

That said, I disagree with this point though:

I propose that a single namespace should be used by each package

Personally I like leaving VS/ReSharper to deal with the proper suggestions, and the fact that when you have multiple namespaces you get a much cleaner IntelliSense window, with only the APIs you're actually working with being displayed on top. It helps a lot especially when you have many APIs in scope, and it just looks cleaner in general.

Additionally, the namespaces often do indicate some conceptual categorization for APIs. Like, I could see all the controls in the Microsoft.Toolkit.Uwp.UI.Controls package being in that root namespace, because they're - well - controls. But if eg. you take the base package, the Guard APIs being in a separate namespace than eg. the collection interfaces makes perfect sense. Or, the HighPerformance having different namespaces for extensions, or buffers, makes sense too. That's the same categorization used in the BCL as well.

I mean, just try to use any API in the BCL right now - there's lots of different namespaces there, and it makes perfect sense. It's not like eg. the whole System.Runtime directory has a single namespace, there's ton of them. Same for other namespaces like System.Buffers - they didn't just throw all the new buffer types directly into System.

I think we should follow the same approach here.

Also to quickly touch upon the MVVM package in particular - I think keeping the separation helps both in mirroring the same namespaces from the BCL, as well as reinforcing the idea that we have loosely coupled components that can be used independently. If you don't need them, just don't include the namespace and you won't even be bothered by those other types in IntelliSense at all. That's why eg. I wouldn't personally want to use a single namespace in that package, for instance.

michael-hawker commented 4 years ago

Thanks @mrlacey! This is definitely something we should address alongside the dependency clean-up already planned for 7.0. I've added this issue for the namespace discussion specifically to our planning issue.

I definitely think there's some opportunities for things like Panels to move to their own package, though they may also live under the Controls namespace as they are generally all used together at times (at least in the system XAML they're all part of the same namespace).

It'd be good to get some input on best practices from a few different places. Do you know if any other OSS projects like .NET itself have any of these guideline documents already?

In the meantime, if you want to look for/list any other spots of inconsistency here, that'd be immensely helpful.

Thanks! 🦙❤

mrlacey commented 4 years ago

I definitely think there's some opportunities for things like Panels to move to their own package, though they may also live under the Controls namespace as they are generally all used together at times (at least in the system XAML they're all part of the same namespace).

Redefining what goes in separate packages should probably be a separate issue as there's lots of opportunity for discussion about what should go where. Do you have real-world usage data to help back up decisions about what should go where? For example, are there lots of people only using "Panels" and no other controls? or is there another benefit of putting these in a separate package?

jamesmcroft commented 4 years ago

Namespaces are definitely important and some packages definitely won't warrant everything being bundled under the same namespace. It defines a clear separation between components and ultimately allows developers to access the bits they need from those.

Obviously for some parts and this leads into a different convo, is having smaller more discrete packages for a more "pick and choose" approach which reduces the size of apps or packages by allowing developers to only pull in the bits they need. That obviously helps reduce the number of namespaces in a single project.

The downside to that, of course, is knowing exactly what is in each package so you can make the decision to take the dependencies.

jamesmcroft commented 4 years ago

I think looking at the associated PR which was closed around the new MVVM package, I think there are some cases where it might make sense for things to be less separated out.

For example, ObservableObject and ObservableRecipient are core to the MVVM toolkit. You might expect those at the root. For the rest though, these are what I'd class as additional components. You've no need to use Messenger, IoC, or the commands if you don't need to. These being clearly separated by namespaces makes a lot of sense.

I could probably see the change in the additional namespace for messages under messaging as being a little confusing as all those components related to messaging in general.

There's no right or wrong way to this though. That being said, I definitely wouldn't create an application and have every class in it at the root namespace.

mrlacey commented 4 years ago

It'd be good to get some input on best practices from a few different places. Do you know if any other OSS projects like .NET itself have any of these guideline documents already?

.NET Framework design guidelines for namespaces

However, acknowledging that rules for a large framework don't automatically apply to a much smaller library.

StackOverflow has lots of questions asking about how to structure/name namespaces and almost all answers point back to the framework guidelines.

The only projects I can find with any specific rules about namespaces in contribution guidelines either say "put everything in one namespace" or "There are only two namespaces (X & Y.) Us X for xxx and Y for yyyy."

I suspect that this project is in a unique position regarding this problem as multiple contributors have done different things over periods of time and no-one has looked at addressing/standardizing this previously.

Interesting note: WinUI only uses 8 namespaces.

Sergio0694 commented 4 years ago

"For example, ObservableObject and ObservableRecipient are core to the MVVM toolkit. You might expect those at the root."

@jamesmcroft The rationale here was to closely mirror the BCL namespaces to reflect the fact that we're providing "reference implementations" for the interfaces there. I've mentioned this in a previous reply to @mrlacey, in general it goes like this:

Additionally, I think having all the components in a separate namespace would reinforce the idea that this library is really pick and choose. That's to say, some devs might very well want to use it just for eg. the Ioc class, or the commands. Having the ObservableObject etc. classes right in the root might instead suggest that you have (or should) use them, and that just the other components are really meant to be optional, which is not the case. Again this is my personal take on this, hope it makes sense! 😊

That said, I guess I could see an argument being made to move the messages into the parent namespace, even though I really do like the additional separation there. The way I see it, the main Messaging namespace includes functionality (so the messenger itself, the interface, and the extensions), while the sub-namespace includes data items, ie. the message themselves. Which in turn are optional - you might very well want to just use the messenger with your own custom messages, in which case you wouldn't want all those extra types popping up in IntelliSense right next to the APIs you actually want to use.

@mrlacey As far as examples from other large projects, the repo I often take inspiration from when organizing code is ImageSharp, which I think is simply one of the best .NET codebases in existance, it's just incredible. You can see that they have a very namespace heavy approach too, which closely (almost perfectly) mirrors the folder structure too. I've been contributing to that repo since last year and I've followed many of the issues and conversations there - so far I don't recall ever seeing anyone complaining about having too many namespaces there.

I'd say especially with even vanilla VS being so good at automatically including namespaces today, there shouldn't really be a problem with this either, and I much rather prefer having more separation in the included APIs when adding a reference 👍

mrlacey commented 4 years ago

I mean, just try to use any API in the BCL right now - there's lots of different namespaces there, and it makes perfect sense. It's not like eg. the whole System.Runtime directory has a single namespace, there's ton of them. Same for other namespaces like System.Buffers - they didn't just throw all the new buffer types directly into System.

I'm not convinced your above arguments apply here.

Sergio0694 commented 4 years ago

"What makes sense for a large framework such as the BCL doesn't automatically apply to a small library"

I never claimed it automatically applies to a small library, I said I think it makes sense for these toolkit packages 😊

And yeah I know some of the System APIs are distributed in separate packages, but even them many of those include APIs from a number of different namespaces, as they directly map to the original APIs in the BCL. To make an example, the System.Memory package includes APIs from System, System.Buffers, System.Runtime.CompilerServices.Unsafe, etc. Same for other packages that can also be installed separately.

That said, I'm not trying to argue that it's a good idea to do this in the toolkit just because the BCL does something similar. I did mention other points to explain why I think it makes perfect sense for us to have more namespaces in many toolkit packages.

In general I don't think we should prioritize "convenience" in having direct access to all the APIs in exchange for a better conceptual separation between APIs, especially with IntelliSense being able to automatically search and add the necessary using directives on its own today. I would say the development experience is already extremely convenient as is thanks to the way IntelliSense and autocompletion work, and this way you also get better code separation on top - the best of woth worlds in my opinion 👍

mrlacey commented 4 years ago

I think there are two potentially conflicting positions that are being argued for here.

  1. What's best for (or preferred by) the authors of the toolkit code.
  2. What's best for the people consuming the toolkit libraries in their apps.

In an ideal world these align but that's not always the case.

Having lots of knowledge about the code (due to writing it) makes it incredibly different to imagine the consumption of the APIs by someone not familiar with it.

How does "a better conceptual separation between APIs" benefit the people using the library? You say it's a good thing (and at an abstract level I agree) but how is it good for the consumers of the library?

In general I don't think we should prioritize "convenience"

😱 😱 😱 😱 😱 😱 😱 😱 I absolutely think the opposite! The toolkit exists to provide "helpers" and "simplify" development. "It just works" should be a goal. It's not a goal to force developers to follow (or event know) specific development patterns or practices. It is not a goal to force people to understand the underlying structure or how classes relate to interfaces in the BCL.

My initial experience porting existing code to the new framework was that it wasn't a lot of effort to do it but there were lots of times where it didn't "just work". Within each class I had to add two or three namespaces but they were all related to using the MVVM library. There were plenty of times where my inner monologue was: "Why doesn't it know about RelayCommand here?" "Oh, I need to add another reference." "But I'm in a ViewModel that references ObservableObject already" "Oh, it's a different namespace" "I guess I'll just let it add what it suggests."

There was no "it just works" I had to rely on the tooling to make it work.

Having the authors of the toolkit code ensure that classes are loosely coupled by using multiple namespaces is one thing. But once the code is written, the existence of multiple namespaces does nothing to enforce the coupling of classes. It just means that there are more namespaces that the consuming code needs to reference. Doesn't it?

Having lots of using directives in a file is a code smell that indicates a file (and the class it contains) may be doing lots of different things. In turn this suggests the code might be better being divided up because it's trying to do too much. Adding lots of using directives that are all related to a narrowly defined area makes the code look more complicated than it is.

The arguments for multiple namespaces being good/ok because the VS tooling can suggest missing namespaces seem contradictory.

Sergio0694 commented 4 years ago

Not to nitpick, but I think you're strawmanning my position here:

In general I don't think we should prioritize "convenience"

😱 😱 😱 😱 😱 😱 😱 😱 I absolutely think the opposite! The toolkit exists to provide "helpers" and "simplify" development. "It just works" should be a goal.

My use of quotes around "convenience" was deliberate, but you replied to that as if I hadn't used them at all.

I did not say that we should not prioritize convenience, but that we should not prioritize "apparent" convenience (hence the quotes). What I mean by this is - you can already access all the types anyway. IntelliSense will already search and suggest all the types across all the existing namespaces in each referenced assembly regardless of the namespace they're from. You literally only need to press on key for autocomplete to kick in, type the rest of the name and also add the using directive for you.

In addition to this, you also get:

And I really, really value this conceptual separation here, especially when you're working in projects that have a number of package references in them. If each package just puts everything in the root namespace because it's "convenient" (in quotes), then the IntelliSense window quickly gets out of hand as you start adding even just a few different using directives. And then again, even just the logical separation of categories just makes sense on its own too, for a number of reasons.

As to your experience porting code, you did mention that your case was pretty unique working on the template studio, having to deal with an incredible number of configurations and a very peculiar template generation system. I don't think that reflects the typical use case for the toolkit. Eg. just as an example, I mentioned I ported my app and it was a very easy and painful transition - the different namespaces didn't really cause any slowdown at all.

mrlacey commented 4 years ago

@Sergio0694 Given that you're the author of the MVVM package you get the final say in how it's structured. I'll agree to differ with your preferences. In terms of the wider toolkit, are there any broad guidelines that can be applied, or should this issue just be about addressing inconsistencies and anomalies in the current codebase?

Sergio0694 commented 4 years ago

Just to reiterate, while I disagree with you on some points mentioned, I do appreciate the conversation! 😊

And I absolutely agree on fixing all the various inconsistencies/anomalies across packages, for starters 👍

Nirmal4G commented 4 years ago

My suggestion is that we should follow the existing Windows Runtime APIs naming conventions. There should be internal docs on best practices. Since Windows Toolkit extends UWP, I think opening up the dev docs within Project Reunion and extend them here as per our needs.

No need to reinvent the wheel. Personally, we should straight up ask either @terrajobst or Framework Design Guidelines for help. 😉

Kyaa-dost commented 3 years ago

Team, Since the deadline of 7.0 is approaching have we decided which route we want to proceed with the namespaces? Based on the convo above do we still need more time?

michael-hawker commented 3 years ago

Thanks @Kyaa-dost, I think @Sergio0694 and I have been learning from reviewing the Animations work that, at least for anything XAML related, having singular namespaces grouping a wide-variety of components is handy as it allows easy discovery of options within a package from a single xmlns namespace.

So for instance, many of our animations helpers (even in other packages) we're providing within the Microsoft.Toolkit.Uwp.UI.Animations namespace so they'll be included when another package is added and add benefit to that ecosystem.

This is similar to the work we're doing in #3594 to split-out the Controls package, but they'll all remain in the Microsoft.Toolkit.Uwp.UI.Controls namespace so they're easier to consume within XAML.

I think @mrlacey's initial comments about some opportunities within specific helpers we should investigate addressing for 7.0. I'm pretty sure we'll be trying to not carry the Services package forward... 🤞 And the Parser should be deprecated, so it's probably not worth us changing anything there; however, we could tweak the other helpers and extensions.

mrlacey commented 3 years ago

Ok. Reviewing this again, I propose the following changes (of the biggest issues):

I've not looked at DataGrid, DesignTools, Parsers, Services, HighPerformance, or the MVVMToolkit. I've also not proposed changing anything that isn't public as the original intention was to standardize the public API.

If no objections are voiced I'll raise a PR

michael-hawker commented 3 years ago

Thanks @mrlacey, a PR sounds wonderful! FYI @andrewleader for the Notifications suggestion.

I think my only thing to comment on is the RemoteSystemKindToSymbolConverter. Even though it's public, it's really an internal thing we're using within the RemoteDevicePicker control only, so I think we intentionally obscured the namespace here to not have it pollute the intellisense when the Controls package is used. It's also not meant to be a general helper and thus why it's not with the rest of the Converters. Any other suggestions for how to handle this scenario?

As for the other packages:

mrlacey commented 3 years ago

I think my only thing to comment on is the RemoteSystemKindToSymbolConverter. Even though it's public, it's really an internal thing we're using within the RemoteDevicePicker control only, so I think we intentionally obscured the namespace here to not have it pollute the intellisense when the Controls package is used. It's also not meant to be a general helper and thus why it's not with the rest of the Converters. Any other suggestions for how to handle this scenario?

Let's leave it as it is then.

As for the other packages:

  • DesignTools are specific to helping with VisualStudio, so we shouldn't have issues there, ideally we auto-generate them in the future with some attributes we place on the controls package. FYI @Nirmal4G

Yeah, didn't see a value for reviewing a lot of files.

  • Parsers and Services are being deprecated and on the way out anyway

That's why I left them.

  • @Sergio0694 want to do another audit of HighPerformance and MVVMToolkit? I know for the Toolkit you were mapping to the other system types, thoughts on keeping that pattern vs. simplifying for usage?

We've had this conversation before. I'm happy to leave with what Sergio decided.

  • DataGrid will eventually move to WinUI, not sure if we want to mess with it now in the meantime, FYI @anawishnoff @RBrid.

Yeah, I just didn't want to go near this large amount of code as it's eventually going to be replaced.

mrlacey commented 3 years ago

Aaah! Docs! 😱

My original intention with this issue was to simplify and standardize namespaces to make things easier for developers consuming the toolkit. However, to make these changes will affect docs. Because the docs URIs are based on namespaces, changes will lead to (potentially) broken docs links.

This presents two options:

  1. Make things simpler for developers in the future but break existing docs links.
  2. Keep a slightly more complex set of namespaces and don't break existing docs links.

Given this choice, I'm now drawn towards not making any changes. Breaking something should only be done when there's a clear benefit. Here the benefit is marginal.

What a big waste of (my) time. sorry. 😞

Sergio0694 commented 3 years ago

Just wanted to post a small update here - following @mrlacey's suggestion I've made some small changes to the HighPerformance package in #3687 to move all the new Memory2D<T> and Span2D<T> types from Microsoft.Toolkit.HighPerformance.Memory to just Microsoft.Toolkit.HighPerformance. I like this change for multiple reasons:

These types are all new in 7.0, so there's no breaking change involved, nor potential issues with the docs either 🙂

On a related note - lately I've been thinking a bit about the various extensions in the toolkit in particular. @mrlacey already pointed this out in the first post in this issue, and I agree that that inconsistency is a bit weird, especially now that newer packages have started to just place them in their respective root namespaces. Basically we have the following situation:

*this is an oversight from #3726, I'll open a PR to fix this since that's a whole new package anyway.

**there's a couple classes in a subfolder with extensions in .Media.Extensions, but I don't think those should be public at all. I would recommend just making a quick PR to make them internal as well like all the other helpers - consumers shouldn't use those.

I guess the question is - other than those two small bits marked with * that I think we should definitely fix in 7.0, do we just want to stick with these inconsistencies then to avoid breaking the docs, or should we at least align the namespaces for extension classes in the Toolkit now, given that the 7.0 release as a whole will be almost completely breaking consumers anyway? I mean, given the huge number of breaking changes in this release it seems as good a time as any to at least rectify this specific bit? 🤔

Thoughts? 🙂