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

implement Zero for Quantity when the backend supports Zero #41

Closed
wants to merge 1 commit into from

Conversation

radix
Copy link
Contributor

@radix radix commented Dec 23, 2017

Here we go, my first try at a trivial trait implementation for uom.

Please let me know if there are any problems or improvements I can make!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.552% when pulling ecaab25 on radix:zero into e05dab3 on iliekturtles:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.559% when pulling 3fa3c46 on radix:zero into e05dab3 on iliekturtles:master.

@iliekturtles
Copy link
Owner

Implementation looks good. Some minor changes then I'll merge!

  • Squash the commits. Change the message to something like "Implement num::Zero.\n\nPart of Add zero constants/methods for each quantity #26."
  • Move the implementation in system.rs below RemAssign.
  • tests.rs
    • Put a blank line beneath let z = ...
    • Use Test::assert_eq(V::Zero, z.value);

@radix
Copy link
Contributor Author

radix commented Dec 23, 2017

FYI I believe you can squash the commits as part of merge with github while you're merging it, though it may be a setting that needs to be enabled in your repository. Regardless I will squash as you requested :)

@radix
Copy link
Contributor Author

radix commented Dec 23, 2017

hooray, forgot to run git add before git commit --amend ;P this test pass should hopefully work...

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.559% when pulling b45948b on radix:zero into e05dab3 on iliekturtles:master.

@iliekturtles
Copy link
Owner

Replaced literal 0.0 with V::zero() and merged. Thanks!

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