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

Resolve test projects' namespace situation #2311

Closed
jnm2 opened this issue Jul 10, 2017 · 11 comments · Fixed by #2535
Closed

Resolve test projects' namespace situation #2311

jnm2 opened this issue Jul 10, 2017 · 11 comments · Fixed by #2535

Comments

@jnm2
Copy link
Contributor

jnm2 commented Jul 10, 2017

We are fighting uphill against convention and against most contributors' tooling by not namespacing according to folder structure. We're inconsistent which leads to confusion and a bunch of using directives with the name segments in a different order when adding new files.

I propose that we (I'm willing) do a sweep and make everything match the folder structure. Most people are used to working this way. The root namespace in every file will be NUnit.Framework.Tests.

NUnit.Framework.Internal.Tests would become NUnit.Framework.Tests.Internal.
NUnit.Framework.Helpers.Tests would become NUnit.Framework.Tests.Helpers (as an example; we'll actually see this when we get to NUnit.Console).

It's bothered me continually since my first PR here and it bothers new contributors as well. I've had it in my mind for months to open this issue but I put it off since it will obviously create merge conflicts in all open PRs. We need to just pick a time and get 'er done.

@CharliePoole
Copy link
Member

CharliePoole commented Jul 10, 2017

I've been fixing this slowly, and it's probably time to do a sweep.

However, my fixes have been in the opposite direction... toward eliminating I the Tests namespace.

Since almost every test class contains the word Test, the namespace is redundant. I like having the test and product classes share a namespace and naming conflicts haven't been a problem so far.

Merge conflicts can be avoided by looking at what work is in progress and leaving tout of the sweep. Otherwise, just do what I'm doing and fix incrementally.

@jnm2
Copy link
Contributor Author

jnm2 commented Jul 10, 2017

I'm fine with that. With this scheme, the root namespace in every file will be NUnit.Framework.

NUnit.Framework.Internal.Tests would become NUnit.Framework.Internal.
NUnit.Framework.Helpers.Tests would become NUnit.Framework.Helpers (as an example; we'll actually see this when we get to NUnit.Console).

@CharliePoole
Copy link
Member

Probably, the default should be changed right away. I probably should have done that rather than planning to do it when all the individual changes were in, because it would have signaled my intent.

@jnm2
Copy link
Contributor Author

jnm2 commented Jul 14, 2017

Apparently we've been in violation of the coding standards: https://github.com/nunit/docs/wiki/Coding-Standards#file-organization

The Directory hierarchy and Namespace hierarchy should match. For example, if the root namespace for a project is NUnit.Framework, files in the Constraints subdirectory should be in the NUnit.Framework.Constraints namespace.

@CharliePoole
Copy link
Member

@jnm2 Can I say "Duh!" 😄 That's what this issue is about, isn't it?

That said, the particular section of the standard represents the thinking of one guy (me) at a particular time (long ago) and may or may not be what we want now. Do we even have standards now? The core team AFAIK has not yet decided, which would be the first step. I consider us to be tabula rasa at this point.

@jnm2
Copy link
Contributor Author

jnm2 commented Jul 14, 2017

I see a big difference between following a coding standard and changing our files to be more friendly and less aggravating. I'd push for the latter without the former.

We should just keep in mind that whoever changes the root namespace is responsible to refactor the namespaces in all the files.

@CharliePoole
Copy link
Member

CharliePoole commented Jul 14, 2017

Why? The root namespace only affects new files and resources. If we can do things incrementally, I think we are more likely to do it.

@jnm2
Copy link
Contributor Author

jnm2 commented Jul 14, 2017

Because sometimes people add new files and have weird issues with using statements and not being able to resolve their types. Plus it causes a warning in every file for people with ReSharper, and sooner or later VS will do the same.

@CharliePoole
Copy link
Member

@jnm2 You marked this as discussion. I was about to work on it, but Discussion means "don't work on it" so I didn't. Is there more you want to discuss?

@jnm2
Copy link
Contributor Author

jnm2 commented Aug 20, 2017

@CharliePoole I'm on vacation which actually means I'm catching up on work. I apologize for leaving you hanging!

<meta>

You marked this as discussion. I was about to work on it, but Discussion means "don't work on it" so I didn't. Is there more you want to discuss?

No. I was waiting for confirmation that we actually want to do the sweep described by the issue rather than deciding to keep doing it incrementally. I didn't hear whether my answer satisfied your objection and no other team member chimed in. I am shy to move things from discussion because I've apparently been too quick to do that... is there even a right pipeline to use when you read my only two options as commanding "work on it" or "don't work on it" =p

But generally, if I'm arguing in favor of moving an issue from discussion to the backlog, I won't mind in the slightest if you signal your agreement by moving the issue to the backlog or by starting work on it.

</meta>

@CharliePoole
Copy link
Member

Well, there's a bit more context in the use of the "discussion" pipeline than just "do it" or "don't do it". I always look at who felt a discussion was needed and in this case it was you.

I'll take a look at this again the next time I'm looking for a task to work on, unless somebody else has started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants