Skip to content

Generate an enum with one variant for each unit, use that for iteration.#215

Merged
iliekturtles merged 3 commits into
iliekturtles:masterfrom
bheisler:iterate_units
Nov 7, 2020
Merged

Generate an enum with one variant for each unit, use that for iteration.#215
iliekturtles merged 3 commits into
iliekturtles:masterfrom
bheisler:iterate_units

Conversation

@bheisler

@bheisler bheisler commented Oct 21, 2020

Copy link
Copy Markdown
Contributor

This extends the quantity! macro to generate a non-exhaustive enum with one variant per unit. A static slice of the different potential values of this enum is also generated, which is exposed publicly through a units() function, which returns an iterator. This can be used to iterate over the different units.

@adamreichold

adamreichold commented Oct 21, 2020

Copy link
Copy Markdown
Contributor

This indeed much simpler than doing tricks with HList! I think a test case that exercises the API would be nice.

iliekturtles
iliekturtles reviewed Oct 22, 2020
View reviewed changes

@iliekturtles iliekturtles left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR!

Comment thread src/quantity.rs Outdated
Comment thread src/quantity.rs Outdated
Comment thread src/quantity.rs Outdated
Comment thread src/quantity.rs Outdated
Comment thread src/quantity.rs Outdated
@bheisler

bheisler commented Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

I think I've fixed everything.

iliekturtles
iliekturtles reviewed Oct 23, 2020
View reviewed changes

@iliekturtles iliekturtles left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Couple more minor changes. I think the code looks good so far but I have only reviewed through github. I want to do one more review, hopefully this weekend, with the compiler then I think we'll be able to merge.

Comment thread src/quantity.rs Outdated
Comment thread src/si/volume.rs Outdated
@bheisler
Address further review comments.
f754470 
- Remove inline on units()
- Move and rename unit test.
@iliekturtles

iliekturtles commented Oct 27, 2020

Copy link
Copy Markdown
Owner

I haven't forgotten about this, I just didn't finish my review this weekend. I like using $plural for the doc comment and think it would be good to switch struct $unit to use the same value if there is no user-provided comment and use the user-provided comment on the enum.

@iliekturtles iliekturtles merged commit a77ef61 into iliekturtles:master Nov 7, 2020
@iliekturtles

iliekturtles commented Nov 7, 2020

Copy link
Copy Markdown
Owner

Merged! Thanks so much for the PR and sorry for the delays. My quest to use the user-provided documentation on the enum members ended up being a dead-end because you can't use a macro in place of an enum member. I will submit a separate PR to convert struct $unit to use $plural.

@bheisler bheisler mentioned this pull request Nov 7, 2020
Closed
@iliekturtles iliekturtles mentioned this pull request Nov 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iliekturtles iliekturtles iliekturtles left review comments

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

@bheisler @adamreichold @iliekturtles