Skip to content

Add base units to some quantities (first steps) #601

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
Merged
merged 29 commits into from
Mar 1, 2019
Merged

Add base units to some quantities (first steps) #601

merged 29 commits into from
Mar 1, 2019

Conversation

tmilnthorp
Copy link
Collaborator

@tmilnthorp tmilnthorp commented Feb 4, 2019 •
edited
Loading

UnitSystem constructors (#547)

@tmilnthorp
Copy link
Collaborator Author

And no, it doesn't work on WRC 😄 I did it quick. Have to pull code and do locally without WRC to see it.

@angularsen
Copy link
Owner

Probably a good idea to merge in #607 first.

@tmilnthorp
Copy link
Collaborator Author

More of a proof of concept. Not ready to merge :)

@angularsen angularsen changed the title Adding base units to all base quantities, and base units for area as … Adding base units to all base quantities (WIP) Feb 15, 2019
@angularsen angularsen mentioned this pull request Feb 16, 2019
Closed
@tmilnthorp
f11b196 
Merge master into GenericMultiply
@tmilnthorp
b571f03 
Regen after WRC split
@tmilnthorp
24af6da 
Cleanup
@tmilnthorp
Can't handle prefixes right now.
9716fed
@tmilnthorp
690229e 
Add test for SI unit system constructor for Area
angularsen
angularsen reviewed Feb 23, 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.

I just took a fresh look at this, been a while since last time. I now finally get how it's supposed to work when specifying say Feet as default Length unit in a UnitSystem. Nice!
From what I can tell, the major things missing is prefix units and derived units/quantities, is there a complication with derived units or is it simply a matter of adding JSON definitions?

@tmilnthorp
Copy link
Collaborator Author

I can't reply directly to your comments so here we go:

  • DefaultUnit would probably be a better name!
  • The only derived unit is area right now. The others just need adding!
  • Yes it is quite verbose. I should
    • Skip adding any BaseUnits if they're all undefined (make it null)
    • Use the default values or object initializers to skip the undefined base units.
  • We can add something for imperial units, but what I'm not sure. I'm open to suggestions. There's so real imperial system in the sense of default units. Even restricting it to the US customary units has inches, feet, yards, and miles for length! No standard like SI.

@angularsen angularsen mentioned this pull request Feb 25, 2019
Closed
@tmilnthorp
Merge branch 'master' into GenericMultiply
3c8e95b
@tmilnthorp
ea120cd 
Regen
@tmilnthorp
Use Undefined BaseUnits where not specified
748a737
@tmilnthorp
Merge "master" into GenericMultiply
f2fdccc
@tmilnthorp
8c6c283 
Doc fix
@tmilnthorp
14bcdca 
Renaming method to ExistsIn and simplifying. Adding test.
@tmilnthorp
2e02250 
Only use arguments that are not undefined for BaseUnits in UnitInfo.
@tmilnthorp
68b4d2a 
Showing example BaseUnits for Joule (energy)
@tmilnthorp
157e3ac 
Adding BaseUnits for Acceleration units
@tmilnthorp tmilnthorp changed the title Adding base units to all base quantities (WIP) Adding some base units to quantities (WIP) Feb 26, 2019
@tmilnthorp
Copy link
Collaborator Author

tmilnthorp commented Feb 26, 2019 •
edited
Loading

I'm actually pretty happy with this now if you'd like to take a look! Not all of them are there, but we can work on adding derived quantity base units as we wish. There's a lot... 😄

@tmilnthorp
5a4f9d3 
Add null check and add tests for GetBaseUnit
@tmilnthorp
0b224df 
Add null test
@tmilnthorp
c1fd1ca 
Null tests for UnitSystem/BaseUnits constructor
angularsen
angularsen requested changes Feb 27, 2019
View reviewed changes
Copy link
Owner

@angularsen angularsen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Phew, this is a lot to take in. I don't think I grok all of it yet, especially the ExistsIn() logic and how it is used, but I'll let you go through my second impression comments first before I read it again.

@tmilnthorp
9833798 
PR suggestions
@tmilnthorp
3a74576 
PR suggestions
@tmilnthorp
32441d5 
PR suggestions
@tmilnthorp
52eee1d 
Rename IsSubsetOf
@tmilnthorp tmilnthorp changed the title Adding some base units to quantities (WIP) Adding base units to some quantities (first steps) Feb 28, 2019
@tmilnthorp
4e43eb6 
Rename IsSubsetOf
@tmilnthorp
46043ad 
Split tests
angularsen
angularsen requested changes Feb 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.

A few more things and this should be ready to go!

@tmilnthorp
911dcfb 
Only expose quantity constructor for UnitSystem. Move getting of unit for base units to QuantityInfo (easier when working with generic types or static info).
@tmilnthorp
Copy link
Collaborator Author

I will add more tests for QuantityInfo.GetUnitInfoForBaseUnitsSubset before merging

angularsen
angularsen requested changes Feb 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.

You might be in the middle of something here, but here are some comments regardless.

@tmilnthorp
Copy link
Collaborator Author

Ok, I think I got through it all! 🎉

angularsen
angularsen reviewed Mar 1, 2019
View reviewed changes
UnitsNet.Tests/QuantityInfoTest.cs
}

[Fact]
public void GetUnitInfoFor_GivenBaseUnitsWithMultipleMatches_ThrowsInvalidOperationException()
Copy link
Owner

Choose a reason for hiding this comment

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

Good test!

@angularsen
Copy link
Owner

I think so too :-) Looks good to me, great job on this one too!

@angularsen angularsen merged commit 7ae21c6 into angularsen:master Mar 1, 2019
@angularsen angularsen changed the title Adding base units to some quantities (first steps) Add base units to some quantities (first steps) Mar 1, 2019
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.
2 participants
@tmilnthorp @angularsen