Skip to content

QuantityInfo: internalizing the UnitInfo construction #1555

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.

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

Merged
merged 6 commits into from
Jul 25, 2025

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Apr 21, 2025

  • QuantityInfo: internalizing the UnitInfo construction
  • QuantityInfo: introducing a delegate for constructing the quantity (required only for net standard)
  • QuantityInfo: introducing an optional ResourceDictionary
  • QuantityInfo: replacing the TUnit[] with an IReadOnlyCollection<TUnit>
  • UnitInfo: introducing a back-reference to the QuantityInfo (making the QuantityName [Obsolete])
  • IQuantity: added the QuantityInfo<TQuantity, TUnit>, the From(double, TUnit) method and default implementations for the non-generic properties
  • QuantityInfoLookup: added another collection for the quantity by type mapping (replacing the generated code in Quantity.g.s).
  • UnitAbbreviationsCache: ReadAbbreviationsFromResourceFile implemented using the provided ResourceManager (if available)
  • updating the QuantityInfo definitions for all quantities (introducing a concrete class, such as MassInfo) with helpers for creating a derived configuration
  • HowMuch upgraded to IQuantity<HowMuch, HowMuchUnit> (the original QuantityInfo is now abstract)
  • Quantity refactored the Parse / From* methods using the default QuantityParser / QuantityInfoLookup
  • Quantity replaced the ByName dictionary with an IReadOnlyDictionary
  • UnitsNet.csproj / UnitsNet.Tests.csproj: added some (specific) implicit usings

lipchev added 2 commits April 21, 2025 08:32
- QuantityInfo: introducing a delegate for constructing the quantity (required only for net standard)
- QuantityInfo: introducing an optional ResourceDictionary
- UnitInfo: introducing a back-reference to the QuantityInfo (making the QuantityName [Obsolete])
- IQuantity: added the QuantityInfo<TQuantity, TUnit>, the From(double, TUnit) method and default implementations for the non-generic properties
- UnitAbbreviationsCache: ReadAbbreviationsFromResourceFile implemented using the provided ResourceDictionary (if available)
- updating the QuantityInfo definitions for all quantities (introducing a concrete class, such as MassInfo) with helpers for creating a derried configuration
- HowMuch upgraded to IQuantity<HowMuch, HowMuchUnit> (the original QuantityInfo is now abstract)
- UnitsNet.csproj / UnitsNet.Tests.csproj: added some (specific) implicit usings
@lipchev lipchev changed the title QuantityInfo: internalizing the UnitInfo construction (WIP) QuantityInfo: internalizing the UnitInfo construction Apr 21, 2025
@lipchev
Copy link
Collaborator Author

lipchev commented Apr 21, 2025

@angularsen The most breaking thing here should be the change from MassUnit[] to IReadOnlyCollection<MassUnit>: we still have the IReadOnlyList<UnitInfo>, so if anybody want to do a for-loop that's still an option, but in my opinion, having the indexer (backed by a dictionary) is more interesting than having 2 read-only lists, but if you want I could switch it.

Providing a delegate and resource dictionary in the QuantityInfo does look like we're mixing the domain with the infrastructure, but since we want to support nestandard2.0 this was the simplest way to have an all in one definition, that can support both internal and external quantities.

PS The UnitInfo is still missing the conversion expressions, that should be used by the UnitConverter, and we still don't have a way to configure/customize the UnitsNetSetup.Default, but for the rest of the classes (UnitParser, UnitAbbreviationsCache etc)- we're pretty close to my final version.

PS2 The new size is:

2.53 MB (2 661 376 bytes)

which is up from

2.18 MB (2 287 616 bytes)

but that would go down again, when I remove all the RegisterDefaultConversions and the switch expressions:

1.78 MB (1 867 776 bytes)

Comment on lines +107 to +119
/// <summary>
/// Creates a new instance of the <see cref="MassInfo"/> class with the default settings for the Mass quantity and a callback for customizing the default unit mappings.
/// </summary>
/// <param name="customizeUnits">
/// A callback function for customizing the default unit mappings.
/// </param>
/// <returns>
/// A new instance of the <see cref="MassInfo"/> class with the default settings.
/// </returns>
public static MassInfo CreateDefault(Func<IEnumerable<UnitDefinition<MassUnit>>, IEnumerable<IUnitDefinition<MassUnit>>> customizeUnits)
{
return new MassInfo(nameof(Mass), DefaultBaseUnit, customizeUnits(GetDefaultMappings()), new Mass(0, DefaultBaseUnit), DefaultBaseDimensions);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, you might be wondering about this thing here (currently uncovered by tests). It allows us to include, exclude or re-configure certain units (not very useful In the current version, where the conversion expressions are still not available).

Here's how this is covered in #1544 : MassTestsBase.g.cs.

The problem is that this it uses these extensions, (tested here) which, I just realized, are using two generic type parameters, making them susceptible to the bug with ReSharper.

If you think we can Ignore and Continue (and you agree with the rest of the changes so far), then I can add them here.

PS Note that in #1544 I went to even further and (with the use of some dazzling re-directions) was able to make the default Mass.Info configurable from the UnitsNetSetup.DefaultConfigurationBuilder, but that's still ahead of us (should you decide to take the red pill 🤣 ).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI - the re-sharper bug was actually closed (as fixed in the EAP version)- I'm expecting the official version to come out next week or so (according to a post I saw 2 weeks ago).

Copy link
Owner

Choose a reason for hiding this comment

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

ReSharper bug is apparently fixed in 2025.2 EAP 5, 2025.2 EAP 6

@angularsen
Copy link
Owner

angularsen commented Apr 24, 2025

@lipchev just a heads up that this week is crazy for me and I'm traveling this weekend, not sure I'll get around to much

Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

Copy link

github-actions bot commented Jul 1, 2025

This PR was automatically closed due to inactivity.

@github-actions github-actions bot closed this Jul 1, 2025
@angularsen angularsen reopened this Jul 25, 2025
Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

Very nice, I like the overall direction. Mostly minor comments and suggestions that can be addressed later, I tried pushing fixes but don't have permission to push to your fork. Will push a few commits after merging this PR.

/// <summary>
/// Provides detailed information about the <see cref="Acceleration"/> quantity, including its name, base unit, unit mappings, base dimensions, and conversion functions.
/// </summary>
public sealed class AccelerationInfo: QuantityInfo<Acceleration, AccelerationUnit>
{
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a little on the fence on the usefulness of having a concrete class for each quantity's info, and we have 100+ quantities that will add to the binary size.

If it's about syntax, a type alias would achieve much the same.
I don't expect to pass AccelerationInfo around or something consumers of UnitsNet should think about.
For extensibility, it doesn't seem to offer much.

Copy link
Owner

Choose a reason for hiding this comment

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

I gave it a go to refactor to private static factories, with minimal diff. Please take a look.
#1580

I think unless we actually need the concrete type for quantity info, let's just reuse the generics we already have to reduce generated code.

@angularsen angularsen merged commit 48ac5df into angularsen:master Jul 25, 2025
1 check was pending
angularsen added a commit that referenced this pull request Jul 25, 2025
lipchev pushed a commit that referenced this pull request Jul 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants