search

fluentassertions / fluentassertions.json

NewtonSoft.Json extensions for FluentAssertions
Apache License 2.0
72 stars 26 forks source link

Namespace - Just FluentAssertions #28

Open cheng93 opened 5 years ago

cheng93 commented 5 years ago

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.

dennisdoomen commented 5 years ago

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 commented 5 years ago

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

In this case JToken/Value/Object

Or classname

jnyrup commented 5 years ago

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 commented 5 years ago

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

Not sure what you're saying

jnyrup commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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