Feature/nullable constructors

[eriove]

Added static "constructors" for nullable doubles as a convenience. When calling the .FromUnitName() or .From() with a nullable double you now get a nullable unit of measure back.

[eriove]

@anjdreas Did you miss this one when you merged #142 or do you have any questions/objections?

[angularsen]

Sorry for the delay, it's good you follow up. As you can see I had some immediate thoughts and that's why it got a bit postponed. Let me know what you think.

[eriove]

I've now changed to calling the non-nullable functions when the nullable double has a value (decreased the size with 4KB, original size before the nullable constructors was 215KB, new size is 252 KB).

If you know the details about all units it makes sense to test only the ones with different implementation details (I didn't even know that Information used decimal). However by generating tests for all units we increase the likelihood to catch future issues for new or changed units.

Following that argument through I should have used a for-each loop for all different units in the different units of measure. But in some way I agree with you, there are many tests already and it feels like they're not really testing anything. My guess is that the only thing these tests really tests is overload resolution problems. These would only occur if custom constructors were added, that is also an argument for more tests.

As you can see I'm torn. I'll think of it during the day and get back to you.

[angularsen]

I've contemplated the very same thing. I agree with your point about tests protecting future refactors, where some units are start differing from others.
However, as the number of units have grown significantly since I introduced this way of generating tests, I do think we should adapt a new testing strategy so that each test actually serves a purpose, instead of just throwing them in there just in case.

These are my initial thoughts:

1. We generate abstract test stubs that must be implemented with values to check against, to ensure all our conversions have a unit test and that we test round-trip conversion between From() constructors and properties converting to units. This is already in place and I think it serves its purpose well.

2. Any non-generated code, should be accompanied by specifically written test units. This would typically involve all code in CustomCode folder, and we have already done a good effort here, but I'm sure it can be covered better yet.

3. This is the sort of "breaking change" of tetsting strategy, in my mind.
   Common behavior, such as ToString(), Parse() etc. that we currently know are generated uniformly across all generated units, should be written by hand and maybe incorporate at least one unit of each internal type (double vs decimal vs long, but if I recall correctly no units are using long at the moment).
   If we at some point in time change the code in such a way that we no longer can rely on testing just a few units, at that time we can consider adding back generated tests or maybe enumerating the units.

What do you think?

[eriove]

1. I agree. This works really well.

2. Same here, this is the obvious way to go. I would say that the tests are quite good here too.

3. I like "breaking change" in this context. The new approach is a reasonable way of doing tests. In this case I would suggest to group the tests by what they are testing and not by unit of measure. (I.e. all tests for this PR would be in the same test class). This differs from what I've done in my previous PRs but if something breaks it will be easier to see if the broken tests are related or unrelated. 5 broken tests in one class it much more approachable than 5 broken tests in 5 different classes.

[eriove]

Moved tests to the file NullableConstructors.cs. Currently testing Length and Information.

Not sure if the file should be in "CustomCode" or in the project root though. What do you think?

[angularsen]

In the root I think, since it is not testing an implementation in the CustomCode folder.
The tests look good to me, good job!

[eriove]

Thanks. Moved "NullableConstructors.cs". Should be ready to merge now.

[angularsen]

I just noticed this. It probably works already, but I found a post on how to properly test for nullable.

http://stackoverflow.com/a/374663/134761

Do you think we should use that method instead?

[eriove]

The tests captured this, hence the change (I would never have discovered it without the tests). Since there is a test that checks that this work I would leave it as is. We could add m.ReturnType.IsValueType && Nullable.GetUnderlyingType(m.ReturnType) == null but what's there right now works and it seems unlikely that more functions named From are added.

[angularsen]

OK, fair enough. We will leave it as-is.

[angularsen]

Nuget 3.28 is out.