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

Add prefixes for watt-hours and ampere-hours #112

Merged
merged 1 commit into from
Mar 3, 2019
Merged

Add prefixes for watt-hours and ampere-hours #112

merged 1 commit into from
Mar 3, 2019

Conversation

svartalf
Copy link
Contributor

@svartalf svartalf commented Mar 3, 2019

Hey, @iliekturtles !

I'm integrating uom into the battery crate and there are few prefixes for watt-hours and ampere-hours that I really could use but there are missing.

Not quite sure if it is worth to add prefixes smaller than micro- though. Any thoughts?

@coveralls
Copy link

coveralls commented Mar 3, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 96.918% when pulling bf6c60a on svartalf:si-wh-and-ah-prefixes into 33901eb on iliekturtles:master.

iliekturtles
iliekturtles requested changes Mar 3, 2019
View reviewed changes
Copy link
Owner

@iliekturtles iliekturtles left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I only found a couple description issues. Once resolved I'll plan on merging. I can also release a v0.21.1 if there aren't any other changes you need for battery (very cool crate).

src/si/electric_charge.rs Outdated
@@ -36,8 +36,18 @@ quantity! {
@zeptocoulomb: prefix!(zepto); "zC", "zeptocoulomb", "zeptocoulombs";
@yoctocoulomb: prefix!(yocto); "yC", "yoctocoulomb", "yoctocoulombs";

@abcoulomb: 1.0_E1; "abC", "abcoulomb", "abcoulombs";
@petaampere_hour: 3.6_E18; "PA · h", "petaampere_hour", "petaampere hours";
Copy link
Owner

Choose a reason for hiding this comment

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

Remove _ from petaampere_hour and teraampere_hour descriptions.

src/si/energy.rs Outdated
@@ -36,6 +36,17 @@ quantity! {
@zeptojoule: prefix!(zepto); "zJ", "zeptojoule", "zeptojoules";
@yoctojoule: prefix!(yocto); "yJ", "yoctojoule", "yoctojoules";

@petawatt_hour: 3.6_E18; "PW · h", "petawatt_hour", "petawatt hours";
Copy link
Owner

Choose a reason for hiding this comment

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

Remove _ from petawatt_hour and terawatt_hour descriptions.

src/si/energy.rs Outdated
@hectowatt_hour: 3.6_E5; "hW · h", "hectowatt hour", "hectowatt hours";
@decawatt_hour: 3.6_E4; "daW · h", "decawatt hour", "decawatt hours";
@watt_hour: 3.6_E3; "W · h", "watt hour", "watt hours";
@milliwatt_hour: 3.6_E0; "mW· h", "milliwatt hour", "milliwatt hours";
Copy link
Owner

Choose a reason for hiding this comment

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

Add a space before the ·.

src/si/energy.rs Outdated
@decawatt_hour: 3.6_E4; "daW · h", "decawatt hour", "decawatt hours";
@watt_hour: 3.6_E3; "W · h", "watt hour", "watt hours";
@milliwatt_hour: 3.6_E0; "mW· h", "milliwatt hour", "milliwatt hours";
@microwatt_hour: 3.6_E-3; "µW ⋅ h", "microwatt hour", "microwatt hours";
Copy link
Owner

Choose a reason for hiding this comment

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

Switch from ⋅ to ·.

@svartalf
Copy link
Contributor Author

svartalf commented Mar 3, 2019

All done.

There is nothing more I need for battery crate, just the few milliamperes and maybe a couple of milliwatt-hours, so new uom version would be nice; I want to merge uom support asap, before somebody will integrate with the currently available API, which can't be called very descriptive.

Btw, thanks for this crate, I have found one logic bug already while integrating it :)

@iliekturtles
Copy link
Owner

Could you squash your PR into a single commit and then I'll merge. I already started writing up v0.21.1 notes so that will be published shortly.

Great to hear uom helped uncover a bug. Could you link the commit? I'm interested to see what is.

@svartalf
Copy link
Contributor Author

svartalf commented Mar 3, 2019

Oh, no, wait there is a missing space in a mAh

iliekturtles
iliekturtles requested changes Mar 3, 2019
View reviewed changes
src/si/electric_charge.rs Outdated Show resolved Hide resolved
@svartalf
Copy link
Contributor Author

svartalf commented Mar 3, 2019 •
edited
Loading

I'm sorry for this mess.

I think this is impossible to find this commit right now, but as far as I remember, I was multiplying microamperes to voltage in order to get microwatt-hours, while microampere-hours should be used there.
It is hard to tell them apart when all types are u32 :)

@iliekturtles iliekturtles merged commit f8c9239 into iliekturtles:master Mar 3, 2019
@iliekturtles
Copy link
Owner

Thanks again for the PR. v0.21.1 published!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iliekturtles iliekturtles iliekturtles 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.
3 participants
@svartalf @coveralls @iliekturtles