Skip to content

This closes anjdreas/UnitsNet#85 - add more units for energy. #86

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 2 commits into from
Jun 25, 2015
Merged

This closes anjdreas/UnitsNet#85 - add more units for energy. #86

merged 2 commits into from
Jun 25, 2015

Conversation

FabioCZ
Copy link

@FabioCZ FabioCZ commented Jun 10, 2015

No description provided.

@FabioCZ FabioCZ mentioned this pull request Jun 10, 2015

protected override double BritishThermalUnitsInOneJoule
{
get { return 1/1055.05585262; }
Copy link
Owner

Choose a reason for hiding this comment

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

I think you're kind of cheating here, since you basically just copied in the implementation into the test. The idea is to double check that you got it right, by looking up the correct answer for these particular conversions.

For example, I type this in google:
british termal units in 1 joule I get 1 joule = 0.00094781712 british thermal units, so I would put that number in here.

Same for values below.

@angularsen
Copy link
Owner

Other than that, looks good to me. If you can just update the test values and make sure all tests run, I'll merge this in. Leave a comment after you are done so I am notified.

Sorry for not getting to this sooner, it slipped my attention.

Cheers

…re not copied directly from the unit declaration json.
@FabioCZ
Copy link
Author

FabioCZ commented Jun 24, 2015

Thanks for your remarks, I pushed another commit with hopefully more appropriate values for the unit tests.

angularsen added a commit that referenced this pull request Jun 25, 2015
Add more units for energy

BritishThermalUnit
Calorie (kilo)
ElectronVolt
FootPound
Erg
WattHour (kilo, mega, giga)
@angularsen angularsen merged commit d585e55 into angularsen:develop Jun 25, 2015
@angularsen
Copy link
Owner

I just noticed, it says 'unknown' on author of your commits. Not a big deal, but you probably want to check your git config and make sure you have name and email configured.

https://git-scm.com/book/en/v2/Getting-Started-First-Time-Git-Setup

@angularsen
Copy link
Owner

Actually, email is set, but it's missing your name, or you have explicitly set it to 'unknown'.

@FabioCZ
Copy link
Author

FabioCZ commented Jun 25, 2015

I had to switch to a Windows machine (I am normally on OS X) to work on this project, and looks like I didn't configure it properly. Anyways, thanks for the merge, and thanks again for a great library.

@FabioCZ FabioCZ deleted the energy-units branch June 25, 2015 21:29
@angularsen
Copy link
Owner

You welcome. I released the nuget yesterday in case you didn't notice.

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