Fix nullability indicators for the Parse/TryParse overloads #1446
Conversation
…Parser, QuantityParser, Quantity) - remove the invalid nullability on the Parse overloads (UnitParser) - added nulability tests
| [Fact] | ||
| public void TryParse_WithNullAbbreviation_ReturnsFalse() | ||
| { | ||
| UnitParser unitParser = UnitsNetSetup.Default.UnitParser; | ||
| Assert.Multiple(() => | ||
| { | ||
| var success = unitParser.TryParse(null, out LengthUnit unit); | ||
| Assert.False(success); | ||
| }, () => | ||
| { | ||
| var success = unitParser.TryParse(null, typeof(LengthUnit), out Enum? _); | ||
| Assert.False(success); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Are you Ok with this type of testing? A lot of my v6 tests use the same convention (merging only when the method name is the same).
There was a problem hiding this comment.
Absolutely, I have a very low bar for testing schemes 😄 as long as they do the job and the name of the test makes sense and kinda follows the other tests in the same area.
There was a problem hiding this comment.
Just to mention, we're using FluentAssertions at work and I do love that.
The syntax is a breeze, assertion messages way more readable and in this case you would do using var _ = new AssertionScope(); before all the assertions.
I wouldn't mind refactoring all tests over to it if I found surplus energy and time. wink wink, nudge nudge 😆
There was a problem hiding this comment.
Don't know if you've got an AI agent- but given a little bit of preparing the right prompt- it should be able to reformat a whole class without much trouble...
If you want to start us up with an example class, I might try it out (one day).
| @@ -517,7 +517,7 @@ private void GenerateStaticParseMethods() | |||
| /// <example> | |||
| /// Length.Parse(""5.5 m"", CultureInfo.GetCultureInfo(""en-US"")); | |||
| /// </example> | |||
| public static bool TryParse(string? str, out {_quantity.Name} result) | |||
| public static bool TryParse([NotNullWhen(true)]string? str, out {_quantity.Name} result) | |||
There was a problem hiding this comment.
This is of course not necessary (and would probably increase our file size a little) but this is how the IParsable interface has it...
Haven't tried it, but I image we would get no warning in the block below:
if (Mass.TryParse(nullableStringAsInput, out _)){
var firstChar = nullableStringAsInput[0]; // I guess this should be safe access..
}There was a problem hiding this comment.
We should definitely do this, and seeing how widely used this is I don't imagine it increases any binary size in practice - meaning, the nuget is probably already loaded if not already bundled in the target framework.
There was a problem hiding this comment.
Okay, maybe a slight extra overhead in metadata of our methods, but still - worth it.
| @@ -517,7 +517,7 @@ private void GenerateStaticParseMethods() | |||
| /// <example> | |||
| /// Length.Parse(""5.5 m"", CultureInfo.GetCultureInfo(""en-US"")); | |||
| /// </example> | |||
| public static bool TryParse(string? str, out {_quantity.Name} result) | |||
| public static bool TryParse([NotNullWhen(true)]string? str, out {_quantity.Name} result) | |||
There was a problem hiding this comment.
Okay, maybe a slight extra overhead in metadata of our methods, but still - worth it.
UnitsNet/GeneratedCode/Quantity.g.cs
Outdated
| @@ -451,7 +451,7 @@ public static bool TryFrom(double value, Enum? unit, [NotNullWhen(true)] out IQu | |||
| /// <param name="quantityString">Quantity string representation, such as "1.5 kg". Must be compatible with given quantity type.</param> | |||
| /// <param name="quantity">The resulting quantity if successful, otherwise <c>default</c>.</param> | |||
| /// <returns>The parsed quantity.</returns> | |||
| public static bool TryParse(IFormatProvider? formatProvider, Type quantityType, string quantityString, [NotNullWhen(true)] out IQuantity? quantity) | |||
| public static bool TryParse(IFormatProvider? formatProvider, Type quantityType, string? quantityString, [NotNullWhen(true)] out IQuantity? quantity) | |||
There was a problem hiding this comment.
I guess we could add [NotNullWhen(true)] to string? quantityString too.
There was a problem hiding this comment.
Seems you only edited the generated file by the way.
I'll make both changes in StaticQuantityGenerator.
There was a problem hiding this comment.
This appear to have caused a compilation error in the AbbreviatedUnitsConverter - I've pushed a fix on my branch (removing the extra ?), but I don't think the PR can be reopened..
…ild failure) (#1457) fixing the nullability of the `unitAbbreviation` in the Parse method (following the build failure from #1446 ) BTW, note that this method isn't even used anywhere. I'm not sure if we've ever used it and then stopped or if it was meant to serve another purpose (having _extended_ the converter)..
(UnitParser, QuantityParser, Quantity)
fixes #1445