Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Loosen bounds in pick_appropriate_units #36

Merged
merged 3 commits into from
Jan 12, 2021
Merged

Loosen bounds in pick_appropriate_units #36

merged 3 commits into from
Jan 12, 2021

Conversation

alberdingk-thijm
Copy link

Change strict inequality to inequality for the inner loop of
pick_appropriate_units, so that if a value is set to 1.0, then
the unit returned follows a user's expectation.
Closes #24.

Change strict inequality to inequality for the inner loop of
pick_appropriate_units, so that if a value is set to 1.0, then
the unit returned follows a user's expectation.
Closes #24.
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!
Could you add some unit tests for the behaviour?
Could you add an entry to the changelog?

@alberdingk-thijm
Copy link
Author

@eldruin added the tests and changelog entry! Let me know if they conform to the project's style; happy to make edits.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Thank you for the changes!
I added some nitpick comments. The most important for me would be more descriptive test names.
Other than that looks good to me.
In case you want to play around with macros, the code could also be simplified with something like this:

macro_rules! test_from_kg {
    ($name:ident, $value:expr, $unit:expr) => {
        #[test]
        fn $name {
            let mass = Mass::from_kilograms($value);
            let (unit, v) = mass.get_appropriate_units();
            assert_eq!(unit, $unit);
            assert_almost_eq($value, v);
        }
    };
}

Use more descriptive test case names, and add a little macro to make
writing the tests less repetitive, per @eldruin's suggestions.
@alberdingk-thijm
Copy link
Author

Whoops, sorry for letting this languish. Thanks for the review, @eldruin, I've made the changes you suggested.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks great to me. Thank you!

@eldruin eldruin merged commit 934959e into rust-embedded-community:master Jan 12, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'pick_appropriate_units()': precision versus prettiness
2 participants