Skip to content

Conversation

@lipchev
Copy link
Collaborator

@lipchev lipchev commented Apr 20, 2025

  • QuantityParser: making the constructor consistent with that of the UnitParser
  • hiding the boxing TryParse overload
  • updating the nullability annotations

…nitParser`

- hiding the boxing TryParse overload
- updating the nullability anotations
@lipchev
Copy link
Collaborator Author

lipchev commented Apr 20, 2025

@angularsen Sorry, I should have done the reformatting before doing this PR..

Anyway, the key points are the change of the constructor: the reason why I've changed it is because I don't think there is any point in instantiating a new QuantityParser (with a new UnitParser) just to use the default abbreviations. I think that creating a new QuantityParser only makes sense if you wanted to provide something custom (either a specific UnitParser or a UnitAbbreviationsCache). Of course, if you think that doesn't outweigh the risk of somebody actually creating an instance with null (or empty), then we could perhaps add an [Obsolete] empty constructor?

The second breaking change is regarding the TryParse overload that returns an IQuantity (even though the delegate is for TQuantity). We're currently using it in the Quantity,TryParse where the return type happens to be an IQuantity, however given that it has the same signature as the generic one, it's very easy to mix them up (costing you an unnecessary boxing of the result). Furthermore, the fact that the two functions are ambiguous, makes it impossible to use var in the result.
Once again- we could have the overload marked as [Obsolete("The result of this method is always boxed, consider using the strongly typed overload.")] but then our own (semi-legitimate) use will blow up with obsolete usage warnings (note that we won't be needing this function at all, once my planned refactoring of the QuantityInfo goes through).

@angularsen angularsen merged commit b71a4bf into angularsen:master Apr 20, 2025
1 check was pending
@angularsen
Copy link
Owner

Looks good to me

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