-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Review namespaces and folder structure coupling #3422
Comments
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! |
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:
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 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 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. |
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 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! 🦙❤ |
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. |
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. |
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. |
.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. |
@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 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 @mrlacey As far as examples from other large projects, the repo I often take inspiration from when organizing code is 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 👍 |
I'm not convinced your above arguments apply here. |
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 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 👍 |
I think there are two potentially conflicting positions that are being argued for here.
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?
😱 😱 😱 😱 😱 😱 😱 😱 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". 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. 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. The arguments for multiple namespaces being good/ok because the VS tooling can suggest missing namespaces seem contradictory.
|
Not to nitpick, but I think you're strawmanning my position here:
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. |
@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. |
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 👍 |
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. 😉 |
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? |
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 So for instance, many of our animations helpers (even in other packages) we're providing within the This is similar to the work we're doing in #3594 to split-out the Controls package, but they'll all remain in the 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. |
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. If no objections are voiced I'll raise a PR |
Thanks @mrlacey, a PR sounds wonderful! FYI @andrewleader for the Notifications suggestion. I think my only thing to comment on is the As for the other packages:
|
Let's leave it as it is then.
Yeah, didn't see a value for reviewing a lot of files.
That's why I left them.
We've had this conversation before. I'm happy to leave with what Sergio decided.
Yeah, I just didn't want to go near this large amount of code as it's eventually going to be replaced. |
Aaah! Docs! 😱 My original intention with this issue was to simplify and standardize namespaces to make things easier for developers consuming the toolkit. This presents two options:
Given this choice, I'm now drawn towards not making any changes. What a big waste of (my) time. sorry. 😞 |
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:
.UI.
form the namespaceThe 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.
The text was updated successfully, but these errors were encountered: