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.

Sign up for GitHub

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

Jump to bottom

Dynamic unit conversions #588

Merged
merged 29 commits into from
Jul 18, 2019
Merged

Dynamic unit conversions #588

merged 29 commits into from
Jul 18, 2019

Conversation

tmilnthorp
Copy link
Collaborator

@tmilnthorp tmilnthorp commented Jan 25, 2019 •
edited by angularsen

  • UnitConverter: Change from hard coded switch statement to lookup of conversion functions
  • Allow consumers to add their own conversion functions between their own quantity types

Idea for #478. I'll add more commentary there.

@tmilnthorp tmilnthorp mentioned this pull request Jan 25, 2019
Closed
@tmilnthorp
Copy link
Collaborator Author

This isn't completely fleshed out yet. Just to show an idea, so don't merge it yet.

Comments in #478

angularsen
angularsen reviewed Jan 26, 2019
View reviewed changes
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.

Interesting approach, it definitely unlocks the ability to add explicit unit-to-unit conversions even across quantities. I'll comment further in the issue #478 instead.

UnitsNet/UnitConverter.cs Outdated Show resolved Hide resolved
UnitsNet/GeneratedCode/Quantities/ElectricInductance.NetFramework.g.cs Outdated Show resolved Hide resolved
angularsen
angularsen reviewed Jan 28, 2019
View reviewed changes
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.

New changes looks good, but still need to digest how and why this will be used. Continuing discussion in issue.

...t.WindowsRuntimeComponent/GeneratedCode/Quantities/Acceleration.WindowsRuntimeComponent.g.cs Outdated Show resolved Hide resolved
UnitsNet/GeneratedCode/UnitConverter.g.cs Outdated Show resolved Hide resolved
UnitsNet/UnitConverter.cs Outdated Show resolved Hide resolved
@angularsen
Copy link
Owner

Adding a summary from prior discussion here:

As for direct conversions, I think that sounds like a good idea regardless:

  1. A single piece of code to handle all conversions, generically
  2. Route existing unit-base and base-unit conversions through it
  3. Ability to add higher-precision direct conversions
  4. Ability to add cross-quantity conversions
  5. Ability to plug in 3rd party conversions of 3rd party unit enums
  6. Compatible with the new dynamic Quantity in Improve working dynamically with units and quantities #576 for converting dynamically

@tmilnthorp tmilnthorp added this to the ln3@Dash milestone Feb 4, 2019
@angularsen angularsen mentioned this pull request Feb 7, 2019
Merged
@angularsen angularsen removed this from the ln3@Dash milestone Feb 13, 2019
@angularsen
Copy link
Owner

Re-reading this now.

@angularsen
Merge remote-tracking branch 'origin/master' into tmilnthorp-UnitConv…
c9e6f46 
…ersions
@angularsen
Use IQuantity.Value in test case
beedad4
@angularsen
Rename typeparams, remove empty ctor
bc3a808
@angularsen
Micro optimization of lookup of IQuantity type
619de36 
It was looked up and wrapped N times
@angularsen
UnitConverter: Move static field initialization to static ctor
6a2befd 
They became a bit unwieldy.
Move fields to top of class.
@angularsen
Add typed conversion function for same quantity conversion functions
1dc6c27 
Since this is the most commonly used call path by far, it made sense to handle this case separately.
The advantage is that the callback quantity is strongly typed.
@angularsen
Use typed conversions in RegisterDefaultConversions
b27f3d5
angularsen
angularsen reviewed Feb 23, 2019
View reviewed changes
Copy link
Owner

@angularsen angularsen left a comment •
edited

Choose a reason for hiding this comment

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

I think this is looking pretty good. I just took the liberty of adding some changes directly to the branch, please review them and see what you think.

https://github.com/angularsen/UnitsNet/pull/588/commits

UnitsNet.Tests/UnitConverterTest.cs Outdated Show resolved Hide resolved
UnitsNet/GeneratedCode/UnitConverter.g.cs Outdated Show resolved Hide resolved
UnitsNet/UnitConverter.cs Outdated Show resolved Hide resolved
@angularsen
Copy link
Owner

When this is merged, this wiki should be updated on the new capability to add unit conversions:
https://github.com/angularsen/UnitsNet/wiki/Extending-with-Custom-Units

@tmilnthorp
Copy link
Collaborator Author

More work to do here (including doc obviously after warnings-as-errors). Just merging to keep up to date.

@tmilnthorp tmilnthorp changed the title Unit conversions Unit conversions (WIP) Mar 1, 2019
@tmilnthorp tmilnthorp mentioned this pull request Mar 1, 2019
Merged
This was referenced Apr 18, 2019
Closed
Closed
@angularsen
Copy link
Owner

@tmilnthorp I took this for spin locally and added some improvements. I think this is ready to merge now, but waiting for you to check out the latest changes.

I was specifically interested in seeing how we can improve support for third party quantities and units and added some test cases for that. It works pretty well by now, you can parse units, parse quantities and convert between quantities using only custom types. We still enforce the requirement of implementing IQuantity, which I did find awkward when adding an example HowMuch quantity since that interface is pretty bloated and most of it is not used for simple conversions and parsing.

I think there is definitely an opportunity to support simple POCO classes and structs without them having to implement IQuantity, but I didn't want to clutter this PR with that.

If you have time, I'd also love to merge #656 .

angularsen
angularsen approved these changes Apr 22, 2019
View reviewed changes
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.

Looks good to me, waiting for you to review my latest improvements.

@angularsen angularsen changed the title Unit conversions (WIP) Dynamic unit conversions Apr 22, 2019
@angularsen
Copy link
Owner

angularsen commented May 1, 2019 •
edited

This needs to port the powershell changes to the new C# codegen scripts.

Update: Fixed.

@angularsen angularsen mentioned this pull request May 8, 2019
Open
@angularsen
Copy link
Owner

@tmilnthorp I think this one is good to go. Two months ago I made some changes to it and I'd like you to look over them whenever you find the time so we can get this puppy merged :)

@angularsen angularsen mentioned this pull request Jul 18, 2019
Closed
@tmilnthorp
Copy link
Collaborator Author

Looks good to me!

@angularsen angularsen merged commit 25ff826 into angularsen:master Jul 18, 2019
@angularsen
Copy link
Owner

Sweet!

@angularsen
Copy link
Owner

Release UnitsNet/4.30.0 · angularsen/UnitsNet

@angularsen angularsen mentioned this pull request Jul 19, 2019
Merged
@angularsen angularsen mentioned this pull request Sep 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angularsen angularsen angularsen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tmilnthorp @angularsen