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

remove redundant namespaces #3405

Closed
wants to merge 3 commits into from
Closed

remove redundant namespaces #3405

wants to merge 3 commits into from

Conversation

SimonCropp
Copy link
Contributor

There is no value in making the internal directory structure be reflected in the public API namespaces. it actually adds more noise since it often request consumers to add more using in file headers. it obscures apis from intellisense for no value.

@ghost
Copy link

ghost commented Aug 2, 2020

Thanks SimonCropp for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Kyaa-dost August 2, 2020 08:39
@michael-hawker
Copy link
Member

Thanks @SimonCropp for submitting another PR. I know there was some discussion on this topic on Twitter here. It didn't seem like there was a concrete outcome of that discussion?

It could've been good to start this as an issue discussion here instead of a direct PR first.

I know the original intent here was to map to the BCL for the underlying interfaces.

I'd love some more input from some of the folks who've been helping us review this work on this namespace topic and have already been using the library in testing things out.

@mrlacey @tannergooding @deanchalk @jamesmcroft @sonnemaf thoughts?

@mrlacey
Copy link
Contributor

mrlacey commented Aug 3, 2020

I think this comes down to preference.
Some people like more, specific namespaces and some like a single higher-level one.

offer an "à la carte" solution with building blocks to freely pick and choose. Using namespaces helps separate the various modules better.

I'm not convinced that having multiple namespaces makes it easier to use individual pieces.
In the above quote (source) it's not clear what "better" means. Again, I think this comes down to preference.

As there is nothing that requires multiple namespaces for disambiguation, and as the contents of each namespace is small, my preference is towards having a single namespace.

While tooling does make it easy to add multiple namespaces when needed, putting everything in one namespace avoids making this an issue.

A quick review of the existing codebase shows inconsistency in this area. (I was hoping for a precedent. 🤷‍♂️)
Whatever the outcome here, a follow-up task needs to be created to look at applying standardization. Or at least define rules for future additions to the toolkit.

@michael-hawker
Copy link
Member

Thanks @mrlacey for insight here. In terms of precedent, I agree that we should have better guidance for the Toolkit as a whole here. We've grown very organically over time, so we need to catch-up on managing a larger project. That is part of our goals with the 7.0 feature planning here though currently more from a package/dependency stand-point.

If you noticed other examples in your quick search, would you mind calling them out in a new issue? 7.0 would be the time to clean things up across the toolkit, so I can link it in our planning memo.

@SimonCropp
Copy link
Contributor Author

While tooling does make it easy to add multiple namespaces when needed, putting everything in one namespace avoids making this an issue.

"putting everything in one namespace" also minimizes the noise in the C# usings header block

@SimonCropp SimonCropp mentioned this pull request Aug 4, 2020
@SimonCropp SimonCropp closed this Aug 4, 2020
@SimonCropp SimonCropp deleted the redundantNamespaces branch August 4, 2020 12:25
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.

3 participants