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

Implicit cast between Duration and TimeSpan #1365

Merged

Conversation

Muximize
Copy link
Contributor

@Muximize Muximize commented Feb 19, 2024

See #1354 (comment)

One issue is that the operator overloads only work when TimeSpan is the right operand.

I changed the code generation to take this into account, but another option would be to make a breaking change where we just don't support TimeSpan as the left operand at all.

Then users would have to cast explicitly, or for multiplication just reverse the operands.

This would affect 13 operators:

TimeSpan * Acceleration
TimeSpan * ElectricCurrent
TimeSpan * ElectricCurrentGradient
TimeSpan * ForceChangeRate
TimeSpan * KinematicViscosity
TimeSpan * MassFlow
TimeSpan * MolarFlow
TimeSpan * Power
TimeSpan * PressureChangeRate
TimeSpan * RotationalSpeed
TimeSpan * Speed
TimeSpan * TemperatureChangeRate
TimeSpan * VolumeFlow

Of which only 6 are used in tests so I assume are supported in v5:

TimeSpan * KinematicViscosity
TimeSpan * MassFlow
TimeSpan * Power
TimeSpan * RotationalSpeed
TimeSpan * TemperatureChangeRate
TimeSpan * Speed

@Muximize
Copy link
Contributor Author

Hmm, these failing CI tests pass on my machine. Any guidance?

@angularsen
Copy link
Owner

@Muximize
Beyond multiplication, are there any cases of TimeSpan as left hand argument?

I can't think of any, so I'd say let's just make a breaking change and not support it.

For multiplication, they can switch order.
Otherwise, just do an explicit cast or maybe call an extension method .ToDuration() on it.

@angularsen
Copy link
Owner

As for the failing test case, that was my bad in a previous merged PR.
Pushed a fix to this branch.

@Muximize
Copy link
Contributor Author

I completely removed TimeSpan from Duration inference, which is a nice cleanup.

Also removed some tests for TimeSpan as the left operand. They all still have a counterpart that tests the reverse.

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.

Another mistake on my part, I'll fix it.

UnitsNet.Tests/CustomCode/DurationTests.cs Outdated Show resolved Hide resolved
@angularsen angularsen merged commit 240b19d into angularsen:release/v6 Feb 26, 2024
1 check was pending
@Muximize Muximize deleted the implicit-cast-Duration-TimeSpan branch February 26, 2024 09:39
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