SerializerOptions for instanciating YAXSerializer #137
Conversation
|
@304NotModified @NKnusperer Unit test for |
|
nice work! 🎉 I have some suggestions! |
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
=====================================
Coverage 76% 76%
=====================================
Files 57 60 +3
Lines 3743 3760 +17
Branches 760 760
=====================================
+ Hits 2845 2867 +22
+ Misses 681 676 -5
Partials 217 217
Continue to review full report at Codecov.
|
304NotModified
left a comment
There was a problem hiding this comment.
Looks great,
Have some recommendation, but I like the structure so approval for now.
One thing I'm doubting about, the names of classes YAXAttributeName and YAXNameSpace. I would expect something like YaxAttributeNameOptions and YAXNameSpaceOptions. What do you guys think?
| var opt = GetNonDefaultOptions(); | ||
| var ser = new YAXSerializer(typeof(SerializerOptions), opt); | ||
|
|
||
| Assert.IsTrue(ser.DefaultExceptionType == opt.ExceptionBehavior); |
There was a problem hiding this comment.
This isn't really what I meant :)
If the test fails, we will see IsTrue(false) failed , some something like that. Then we can't see on the build server what's wrong and we need to check it locally (same for IsFalse etc)
I would recommend to do:
Assert.AreEqual (ser.DefaultExceptionType, opt.ExceptionBehavior, "ExceptionBehavior"); etc. (not 100% about the syntax, not double checked)
If you don't like writing all those description strings, than FluentAssertions helps on that:
ser.DefaultExceptionType.Should().Be(opt.ExceptionBehavior); // This will give nice DefaultExceptionType in the message
SerializerOptionsYAXSerializer(Type, SerializerOptions)is the only constructor not flagged as obsoleteYAXSerializervariables toSerializerOptionsand markedYAXSerializerproperties as obsolete. These properties now read/writeSerializerOptionsYAXSerializerImplementing
CultureInfoas pointed out in #80 will follow in the next step