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

Determine BaseUnits of Prefix-Unit #1233

Closed
wants to merge 0 commits into from
Closed

Conversation

bdbonazz
Copy link

I found a way to determine come BaseUnits of Units added dinamically by prefix.
The main change is to file "CodeGen\Generators\QuantityJsonFilesParser.cs"
New to Open-Source, Open to criticism.

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.

Hi, this looks interesting. Some comments below.

CodeGen/Generators/QuantityJsonFilesParser.cs Outdated Show resolved Hide resolved
@tmilnthorp
Copy link
Collaborator

So just thinking about this - wouldn't the base units just be the same as the ones without the prefixes? Per your example:

MicroPascal could etiher be "g/ms^2" or "kg/kms^2"

@angularsen
Copy link
Owner

angularsen commented Jun 2, 2023

So just thinking about this - wouldn't the base units just be the same as the ones without the prefixes? Per your example:

I have never used base units in UnitsNet for anything myself, but my understanding of the use case for defining base units is that it makes it easier for an application developer to let the user choose what SI base units to use, then all quantities will automatically be shown in compatible units where possible.

For example, if user selects Centimeter as the SI base unit for Length, then all quantities would be represented using centimeters, such as Area cm², Volume cm³ and so on.

It may not be able to find a unit for quantities not based on length, such as Angle.
It may not be able to find a unique unit for all quantities were multiple units share the same SI base units.

Since I haven't used this in any practical application myself, it all is a bit theoretical for me at this point and not so easy to reason about.

Personally, if I were to offer unit preferences in an application, I would probably create a custom mapping between SI base units and units in the quantities I care about.

However, since we do have base units and maybe it does add some value, then I still think this PR seems worthwhile to merge.

@angularsen
Copy link
Owner

angularsen commented Jun 16, 2023

@bdbonazz Sorry this took a long time.
I'd like to move ahead with merging this, but I noticed a few additional changes that I don't think should be included here.

Please revert/undo these changes then re-generate code

AmountOfSubstance.json
LuminousIntensity.json
Mass.json

They add new units, and this should be a separate pull request from determining base units of prefix units.

Tip for next time

Create a new branch on your fork instead of committing on your master branch, this way you can more easily have multiple features on different branches, and maintainers trying to merge are allowed to make changes to the branch that can help speed up the review process by fixing minor things immediately.

@bdbonazz bdbonazz closed this Jun 19, 2023
@bdbonazz bdbonazz force-pushed the master branch 2 times, most recently from f2963ae to 439c143 Compare June 19, 2023 19:52
@angularsen
Copy link
Owner

@bdbonazz Seems you closed this without creating a new pull request. Do you still intend to complete this?

@bdbonazz
Copy link
Author

@bdbonazz Seems you closed this without creating a new pull request. Do you still intend to complete this?

I am sorry
I changed jobs for a month so I don't have much time left for github
I tried to do what you told me, but in the branch where i reverted the changes and only added some Units, some tests stopped working and I didn't have time to figure out why
I didn't close this PR on purpose, I must have done it by mistake when I reverted the changes

@angularsen
Copy link
Owner

@bdbonazz I fully understand time is hard to come by, no pressure from my end.

The tests that stopped working was probably a broken change in master branch that I recently fixed in #1267 .

If you could update your feature branch to take in the latest changes, I believe you should be good to go. If you push your branch and create a pull request for it, then you have some UI buttons to help update the branch by either rebase or merge.

I'm happy to help if you need assistance.

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.

None yet

3 participants