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

Add liter-based volume units to si #116

Merged
merged 1 commit into from
Mar 24, 2019
Merged

Conversation

Aehmlo
Copy link
Contributor

@Aehmlo Aehmlo commented Mar 21, 2019

This PR adds liter-based (i.e. liter and all prefixes) units to si::volume. The included tests test liter against cubic_meter and then all of the prefixed liter variants against liter to ensure that they are all correct.

@coveralls
Copy link

coveralls commented Mar 21, 2019

Coverage Status

Coverage increased (+0.05%) to 96.968% when pulling 08f9e3c on Aehmlo:volume into 8655f54 on iliekturtles:master.

@iliekturtles
Copy link
Owner

Initial review looks good. Will go over the details tomorrow.

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.

Two minor changes then we should be good to merge. Squash everything into a single commit.

@gigaliter: prefix!(milli) * prefix!(giga); "GL", "gigaliter", "gigaliters";
@megaliter: prefix!(milli) * prefix!(mega); "ML", "megaliter", "megaliters";
@kiloliter: prefix!(milli) * prefix!(kilo); "kL", "kiloliter", "kiloliters";
@hectoliter: prefix!(milli) * prefix!(hecto); "hL", "hectoliter", "hectoliters";
Copy link
Owner

Choose a reason for hiding this comment

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

deca prefix got missed. Add for tests as well.

test::<v::femtoliter, v::liter>(prefix!(femto));
test::<v::attoliter, v::liter>(prefix!(atto));
test::<v::zeptoliter, v::liter>(prefix!(zepto));
test::<v::yoctoliter, v::liter>(prefix!(yocto));
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 blank line just above the function.

@iliekturtles
Copy link
Owner

I see the force push but not the changes. Try again?

@Aehmlo
Copy link
Contributor Author

Aehmlo commented Mar 22, 2019

Odd, seems I forgot to stage. I’ll fix that in a bit here.

@iliekturtles iliekturtles merged commit 372f16e into iliekturtles:master Mar 24, 2019
@iliekturtles
Copy link
Owner

Thanks again!

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.

3 participants