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.

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

Add type safety and improve type inference in UnitsMath.cs #1373

Closed
UrielZyx opened this issue Mar 1, 2024 · 6 comments
Closed

Add type safety and improve type inference in UnitsMath.cs #1373

UrielZyx opened this issue Mar 1, 2024 · 6 comments

Comments

@UrielZyx
Copy link

UrielZyx commented Mar 1, 2024

Is your feature request related to a problem? Please describe.
Some of the extension methods inUnitsMath.cs (e.g. Average) take an Enum unitType argument. Our system uses these extensively, and when copying/pasting code (and changing the quantity type because we're dealing with different sensors every time), I'd like the compiler to remind me I forgot to update the unit.

Describe the solution you'd like
Change

public static TQuantity Average<TQuantity>(
    this IEnumerable<TQuantity> source,
    Enum unitType)
    where TQuantity : IQuantity

into something like:

public static TQuantity Average<TQuantity>(
    this IEnumerable<TQuantity> source,
    TUnitType unitType)
    where TUnitType : Enum
    where TQuantity : IQuantity<TUnitType>

Describe alternatives you've considered
We tried implementing type safe methods in our own codebase, but when we use the wrong unit type, our IDE just falls back to the implementation in UnitsMath.

Additional context

  • This will break backwards compatibility, but only for people doing really weird things.
  • This change might be warranted in a lot of other places that take a unitType parameter (I haven't checked yet, mostly because they didn't come up in my use-case)
@angularsen
Copy link
Owner

This sounds useful, would you be interested in attempting a pull request?

You can target release/v6 branch, where we already have made some breaking changes.

@UrielZyx
Copy link
Author

UrielZyx commented Mar 2, 2024

Sounds great.
By "target" do you mean I should branch off of release/v6 (sorry, I have limited experience with GitHub)?

@UrielZyx

This comment has been minimized.

@UrielZyx

This comment has been minimized.

@UrielZyx
Copy link
Author

UrielZyx commented Mar 2, 2024

PR is ready at #1374
I looked around to see if I can find other places with the same issue. The only one I could find was in UnitConverter.cs, but it breaks generated code, so probably better to keep for a different PR (if at all).

@angularsen
Copy link
Owner

Fixed by #1374

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

No branches or pull requests

2 participants