octokit.net

octokit.net copied to clipboard

At some point a folder called fixtures was introduced. This lead to a number of classes having the namespaceOctokit.Tests.Integration.fixtures

Will make a PR to fix once #1144 is merged (as it introduces some more files to that namespace)

(Affected Files - potentially more)

I would love to remove all namespaces from unit and integration tests. They really don't serve a purpose for our tests. Namespaces are useful for libraries that you reference in order to disambiguate class names. But you never reference a set of unit tests for your own code. At least, we don't need to support that if you do for some wild reason. :stuck_out_tongue:

Would you be open to one fat PR that rips out all namespace parts lower than what matches the project? Or shall I split it up?

Split it up. I worry that one big fat PR will cause problems for everybody else. :)

Just a heads up - removing the namespaces is likely to cause a whole bunch of indenting changes to the test suite. It's a minor nuisance but things like git blame will then be full of lies...

~~What do you mean by indenting changes?~~ Based on my next comment I think I understand what you where saying, @shiftkey

An example of what things look like now is @Haacked do you want to just drop the .Clients or drop the namespace altogether (like :arrow_down: )?

@M-Zuber that's exactly the issue, and why I kinda put it aside when I started finding these 😢

IMO I would do the first option. having a namespace that matches the project name does not create noise (at least to me) - and that is what is included in the default.

Is there a setting that prevents VS from tacking on namespace parts to match the file system?

Is there a setting that prevents VS from tacking on namespace parts to match the file system?

Unfortunately it seems to be R# only

I am voting for leaving a namespace that matches the project name, but dropping any further levels.

Files that currently have no namespace can stay that way. I would also include adding a test to Conventions to the PR.

@shiftkey @Haacked @ryangribble any feedback please?

It's very unfortunate because not even thinking about files where extra namespace levels were added due to folder path, we seem to have a real mix of unit and integration tests where some files have a namespace and others have none. My OCD wants things to be consistent, particularly now it's been brought to attention (:trollface:) , but yeah im not sure if the code/indentation churn is worth it...

I would also include adding a test to Conventions to the PR.

Pretty tough to do when there is inconsistency in the files. Or are you saying the convention test would "pass" if there is no namespace OR a namespace matching project, and would only fail when a namespace was present that didn't match the project?

Yeah I meant the the test should only fail if there is a namespace and it doesn't match the project

So how do we reach a consensus on this, so I can either code or close?

@M-Zuber assigning myself to look at this tomorrow

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!