Skip to content

Replacing the ToString overloads with extensions #1552

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Apr 19, 2025

  • introducing the QuantityExtensions (in the Extensions folder) with the first two extensions for the ToString overloads
  • removed the ToString(IFormatProvider?) overload from the IQuantity interface
  • removed the ToString overloads from the generated quantities (and HowMuch)

@lipchev
Copy link
Collaborator Author

lipchev commented Apr 19, 2025

Ok, here is the size difference:

before: 2.19 MB (2 301 440 bytes)
after: 2.17 MB (2 286 080 bytes)

Since we've only got one generic parameter, the bug with Resharper does not apply.

The only breaking parts concern the cases where a given class doesn't have the UnitsNet namespace import (e.g. when given a type that contains a quantity, which is then accessed without assigning to a variable).

I'll make separate PR, with the other version which simply removes the overloads and makes the parameters optional.

@angularsen
Copy link
Owner

angularsen commented Apr 20, 2025

Yeah, I don't like how default parameters for ToString() make it awkward to only pass a culture. It's just so common to have relevant ToString() overloads for this.
#1553 (comment)

Extension methods could work though, but it does have some potential developer expirence friction as you point out regarding missing namespace imports. I expect this to be fairly uncommon though and it only affects 2 of 4 ToString() overloads, and IDEs may do a decent job of importing namespaces for you.

I guess it comes down to, is the 25 kB binary size reduction worth a potentially worse developer experience? ToString() is used a lot.

I'm kinda leaning towards maybe not enough impact to justify this change, what do you think?
Or we could try it and see how much outcry there is. Maybe very few are impacted.

@angularsen
Copy link
Owner

With the perspective that we want to try and use extension methods more in general to avoid duplicating code for N quantities, maybe we'll just have to try this and see how it feels.

@lipchev
Copy link
Collaborator Author

lipchev commented Apr 20, 2025

For me personally it's just the interface method that's bothering me.. It might look like I'm optimizing for size, but that's really not a much of priority (in my use case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants