Skip to content

Generate operators from unit relations defined in JSON #1329

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

Merged
merged 13 commits into from
Feb 4, 2024

Conversation

Muximize
Copy link
Contributor

@Muximize Muximize commented Nov 13, 2023

Related #1200

In the PR adding generic math (#1164) @AndreasLeeb states:

Regarding the operators in the *.extra.cs files, that could be tackled easily by describing the dependencies (operations) between different quantities in the quantity JSON files, and then the operator overloads and the generic math interfaces for the quantity structs could also be automatically generated. But that's a topic for another time 😄

I decided to give this a shot.

UnitRelations.json contains relations extracted from the existing *.extra.cs files. I decided on a new file because multiplication is commutative and I didn't want to duplicate these in the individual quantity JSON files, or risk missing one or the other, so it's best to define them once in one place. The generator handles this by generating two operators for a single multiplication relation.

The relations format uses the quantities method names. This is a bit unfortunate, but it's the best I could come up with without making the CodeGen project depend on UnitsNet, which would create a bit of a chicken/egg problem. This is not unheard of (self-hosted compilers) but I wanted to keep it simple for now.

The generated code enables the removal of 44 *.extra.cs files, and the 17 remaining contain much less code.

@Muximize Muximize changed the title [WIP] Generate operators from defined unit relations in JSON [WIP] Generate operators from unit relations defined in JSON Nov 13, 2023
@Muximize
Copy link
Contributor Author

@angularsen What's your opinion about this? Is this a worthwhile endeavor in the right direction?

@angularsen
Copy link
Owner

Apologies, I thought this was still being worked on so I haven't looked at it yet.
The PR description sounds very useful, I'm just curious if there are any challenges to this approach or not.

Let me read more through the code and get back to you 👍 Will try to find time the next couple of days.

@Muximize
Copy link
Contributor Author

No worries, I should have been more clear that this is a proposal in need of feedback. God jul! 😄

@Muximize Muximize changed the title [WIP] Generate operators from unit relations defined in JSON Generate operators from unit relations defined in JSON Dec 22, 2023
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.

I think this looks very promising and a big improvement on the existing approach.

A few ideas below.

angularsen pushed a commit that referenced this pull request Jan 2, 2024
Came across this while working on #1329. I'm afraid it's an API-breaking
change, so maybe it should wait for v6?

Related:
#1200
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.

I think this looks solid! This is some impressive work 👏

Some minor suggestions for you to consider.

The biggest one for me is to get some peace of mind that we are not breaking anyone as it is hard to verify by review.

I added a suggestion to maybe use reflection to compare before/after of all quantities and their public members, by outputting to text and diffing that.

@angularsen angularsen changed the base branch from master to release/v6 February 4, 2024 14:10
@angularsen angularsen mentioned this pull request Feb 4, 2024
24 tasks
@angularsen angularsen merged commit 2424307 into angularsen:release/v6 Feb 4, 2024
@angularsen angularsen added this to the vNext milestone Feb 4, 2024
angularsen pushed a commit that referenced this pull request Mar 1, 2024
In
[#1329](#1329 (comment))
this proposal came up:

> Another idea: generate division operators based on multiplication.
Right now we define:
> ```
> ElectricPotential.Volt = ElectricCurrent.Ampere *
ElectricResistance.Ohm (and generate the reverse)
> ElectricCurrent.Ampere = ElectricPotential.Volt /
ElectricResistance.Ohm
> ElectricResistance.Ohm = ElectricPotential.Volt /
ElectricCurrent.Ampere
> ```
> But those last two could also be generated based on the first.

This PR is an experiment implementing this.

### Breaking changes:

- `TimeSpan = Volume / VolumeFlow` => `Duration = Volume / VolumeFlow`
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