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.

Sign up for GitHub

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

Fix nullability indicators for the Parse/TryParse overloads #1446

Merged
merged 2 commits into from
Dec 15, 2024
Merged

Fix nullability indicators for the Parse/TryParse overloads #1446

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3449bc5
- add missing nullability indicators for the TryParse overloads (Unit…
lipchev Dec 10, 2024
1cca84a
[NotNullWhen(true)] string? quantityString
angularsen Dec 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions CodeGen/Generators/UnitsNetGen/QuantityGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, maybe a slight extra overhead in metadata of our methods, but still - worth it.

{{
return TryParse(str, null, out result);
}}
Expand All @@ -532,7 +532,7 @@ public static bool TryParse(string? str, out {_quantity.Name} result)
/// Length.Parse(""5.5 m"", CultureInfo.GetCultureInfo(""en-US""));
/// </example>
/// <param name=""provider"">Format to use when parsing number and unit. Defaults to <see cref=""CultureInfo.CurrentCulture"" /> if null.</param>
public static bool TryParse(string? str, IFormatProvider? provider, out {_quantity.Name} result)
public static bool TryParse([NotNullWhen(true)]string? str, IFormatProvider? provider, out {_quantity.Name} result)
{{
return QuantityParser.Default.TryParse<{_quantity.Name}, {_unitEnumName}>(
str,
Expand Down Expand Up @@ -571,7 +571,7 @@ public static bool TryParse(string? str, IFormatProvider? provider, out {_quanti
}}

/// <inheritdoc cref=""TryParseUnit(string,IFormatProvider,out UnitsNet.Units.{_unitEnumName})""/>
public static bool TryParseUnit(string str, out {_unitEnumName} unit)
public static bool TryParseUnit([NotNullWhen(true)]string? str, out {_unitEnumName} unit)
{{
return TryParseUnit(str, null, out unit);
}}
Expand All @@ -586,7 +586,7 @@ public static bool TryParseUnit(string str, out {_unitEnumName} unit)
/// Length.TryParseUnit(""m"", CultureInfo.GetCultureInfo(""en-US""));
/// </example>
/// <param name=""provider"">Format to use when parsing number and unit. Defaults to <see cref=""CultureInfo.CurrentCulture"" /> if null.</param>
public static bool TryParseUnit(string str, IFormatProvider? provider, out {_unitEnumName} unit)
public static bool TryParseUnit([NotNullWhen(true)]string? str, IFormatProvider? provider, out {_unitEnumName} unit)
{{
return UnitParser.Default.TryParse<{_unitEnumName}>(str, provider, out unit);
}}
Expand Down
23 changes: 22 additions & 1 deletion UnitsNet.Tests/QuantityParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
// Licensed under MIT No Attribution, see LICENSE file at the root.
// Licensed under MIT No Attribution, see LICENSE file at the root.
// Copyright 2013 Andreas Gullberg Larsen ([email protected]). Maintained at https://github.com/angularsen/UnitsNet.

using System.Globalization;
using UnitsNet.Tests.CustomQuantities;
using UnitsNet.Units;
using Xunit;

namespace UnitsNet.Tests
Expand Down Expand Up @@ -87,5 +89,24 @@ public void TryParse_MappedCustomUnit()
Assert.Equal(1, q.Value);
}

[Fact]
public void TryParse_NullString_Returns_False()
{
QuantityParser quantityParser = UnitsNetSetup.Default.QuantityParser;

var success = quantityParser.TryParse<Mass, MassUnit>(null, null, Mass.From, out Mass _);

Assert.False(success);
}

[Fact]
public void TryParse_WithInvalidValue_Returns_False()
{
QuantityParser quantityParser = UnitsNetSetup.Default.QuantityParser;

var success = quantityParser.TryParse<Mass, MassUnit>("XX kg", CultureInfo.InvariantCulture, Mass.From, out Mass _);

Assert.False(success);
}
}
}
15 changes: 15 additions & 0 deletions UnitsNet.Tests/UnitParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,5 +137,20 @@ public void Parse_MappedCustomUnit()

Assert.Equal(HowMuchUnit.Some, parsedUnit);
}

[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);
});
}
Comment on lines +141 to +154
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😆

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
}
2 changes: 1 addition & 1 deletion UnitsNet/CustomCode/QuantityParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public bool TryParse<TQuantity, TUnitType>(string? str,
/// <exception cref="ArgumentNullException">The string was null.</exception>
/// <exception cref="FormatException">Failed to parse quantity.</exception>
[SuppressMessage("ReSharper", "UseStringInterpolation")]
public bool TryParse<TQuantity, TUnitType>(string str,
public bool TryParse<TQuantity, TUnitType>(string? str,
IFormatProvider? formatProvider,
QuantityFromDelegate<TQuantity, TUnitType> fromDelegate,
out IQuantity? result)
Expand Down
10 changes: 5 additions & 5 deletions UnitsNet/CustomCode/UnitParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public TUnitType Parse<TUnitType>(string unitAbbreviation, IFormatProvider? form
/// <returns>Unit enum value, such as <see cref="MassUnit.Kilogram" />.</returns>
/// <exception cref="UnitNotFoundException">No units match the abbreviation.</exception>
/// <exception cref="AmbiguousUnitParseException">More than one unit matches the abbreviation.</exception>
public Enum Parse(string? unitAbbreviation, Type unitType, IFormatProvider? formatProvider = null)
public Enum Parse(string unitAbbreviation, Type unitType, IFormatProvider? formatProvider = null)
{
if (unitAbbreviation == null) throw new ArgumentNullException(nameof(unitAbbreviation));
unitAbbreviation = unitAbbreviation.Trim();
Expand Down Expand Up @@ -138,7 +138,7 @@ internal static string NormalizeUnitString(string unitAbbreviation)
/// <param name="unit">The unit enum value as out result.</param>
/// <typeparam name="TUnitType">Type of unit enum.</typeparam>
/// <returns>True if successful.</returns>
public bool TryParse<TUnitType>(string unitAbbreviation, out TUnitType unit) where TUnitType : struct, Enum
public bool TryParse<TUnitType>([NotNullWhen(true)]string? unitAbbreviation, out TUnitType unit) where TUnitType : struct, Enum
{
return TryParse(unitAbbreviation, null, out unit);
}
Expand All @@ -151,7 +151,7 @@ public bool TryParse<TUnitType>(string unitAbbreviation, out TUnitType unit) whe
/// <param name="unit">The unit enum value as out result.</param>
/// <typeparam name="TUnitType">Type of unit enum.</typeparam>
/// <returns>True if successful.</returns>
public bool TryParse<TUnitType>(string? unitAbbreviation, IFormatProvider? formatProvider, out TUnitType unit) where TUnitType : struct, Enum
public bool TryParse<TUnitType>([NotNullWhen(true)]string? unitAbbreviation, IFormatProvider? formatProvider, out TUnitType unit) where TUnitType : struct, Enum
{
unit = default;

Expand All @@ -169,7 +169,7 @@ public bool TryParse<TUnitType>(string? unitAbbreviation, IFormatProvider? forma
/// <param name="unitType">Type of unit enum.</param>
/// <param name="unit">The unit enum value as out result.</param>
/// <returns>True if successful.</returns>
public bool TryParse(string unitAbbreviation, Type unitType, [NotNullWhen(true)] out Enum? unit)
public bool TryParse([NotNullWhen(true)] string? unitAbbreviation, Type unitType, [NotNullWhen(true)] out Enum? unit)
{
return TryParse(unitAbbreviation, unitType, null, out unit);
}
Expand All @@ -182,7 +182,7 @@ public bool TryParse(string unitAbbreviation, Type unitType, [NotNullWhen(true)]
/// <param name="formatProvider">The format provider to use for lookup. Defaults to <see cref="CultureInfo.CurrentCulture" /> if null.</param>
/// <param name="unit">The unit enum value as out result.</param>
/// <returns>True if successful.</returns>
public bool TryParse(string? unitAbbreviation, Type unitType, IFormatProvider? formatProvider, [NotNullWhen(true)] out Enum? unit)
public bool TryParse([NotNullWhen(true)] string? unitAbbreviation, Type unitType, IFormatProvider? formatProvider, [NotNullWhen(true)] out Enum? unit)
{
if (unitAbbreviation == null)
{
Expand Down
8 changes: 4 additions & 4 deletions UnitsNet/GeneratedCode/Quantities/AbsorbedDoseOfIonizingRadiation.g.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions UnitsNet/GeneratedCode/Quantities/Acceleration.g.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions UnitsNet/GeneratedCode/Quantities/AmountOfSubstance.g.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions UnitsNet/GeneratedCode/Quantities/AmplitudeRatio.g.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions UnitsNet/GeneratedCode/Quantities/Angle.g.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading