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

Namespace - Just FluentAssertions #28

Open
cheng93 opened this issue Apr 15, 2019 · 7 comments
Open

Namespace - Just FluentAssertions #28

cheng93 opened this issue Apr 15, 2019 · 7 comments

Comments

@cheng93
Copy link

cheng93 commented Apr 15, 2019

https://github.com/fluentassertions/fluentassertions.json/blob/master/Src/FluentAssertions.Json/JsonAssertionExtensions.cs#L5

Should the namespace for that file be just FluentAssertions ?

Then we won't even need this message in the docs

Be sure to include using FluentAssertions.Json; otherwise false positives may occur.

@cheng93 cheng93 changed the title Namespace - Just FluentAssertion Namespace - Just FluentAssertions Apr 15, 2019
@dennisdoomen
Copy link
Member

I'm not sure. If we do, you might end up have collisions with the main assembly. Either way, changing this now is going to be a breaking change. cc @jnyrup

@cheng93
Copy link
Author

cheng93 commented Apr 26, 2019

I believe collisions would only occur if the main library has a conflicting overload

In this case JToken/Value/Object

Or classname

@jnyrup
Copy link
Member

jnyrup commented Apr 26, 2019

Some observations:

All FluentAssertions extensions currently use a sub namespace, e.g. FluentAssertions.Json, FluentAssertions.Mvc, etc.

Currently without using FluentAssertions.Json

((JToken)null).Should(); // GenericCollectionAssertions<JToken>
((JValue)null).Should(); // Ambiguous between `ObjectAssertions`, `GenericCollectionAssertions<JToken>` and `ComparableTypeAssertions<JToken>`
((JObject)null).Should(); // `GenericDictionaryAssertions<string, JToken>`

I don't think it would harm to unnest the namespace as a breaking change.

@dennisdoomen
Copy link
Member

I don't think it would harm to unnest the namespace as a breaking change.

Not sure what you're saying

@jnyrup
Copy link
Member

jnyrup commented Apr 26, 2019

@dennisdoomen No, that sentence didn't make an sense.
I don't think moving this extension from FluentAssertions.Json to FluentAssertions would cause major problems, as none of the three types extended on are from BCL and JTokenAssertions extends ReferenceTypeAssertions.
On the other hand, all other FA extensions are in sub namespaces, so I assume they "suffer" from the same problem.

@cheng93
Copy link
Author

cheng93 commented Apr 26, 2019

I think the other repos could also be moved to FluentAssertions namespace.

Note that this is only the extension class containing the Should() methods

@dennisdoomen
Copy link
Member

Yeah, but changing namespace FluentAssertions.Json to FluentAssertions will most definitely cause breaking changes to existing users of this extension library.

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

No branches or pull requests

3 participants