Skip to content

feat(volume_rate): Add cubic_meter_per_min and cubic_meter_per_hour as volume rate units#406

Merged
iliekturtles merged 1 commit into
iliekturtles:masterfrom
Uzaaft:volume_rate
Feb 11, 2023
Merged

feat(volume_rate): Add cubic_meter_per_min and cubic_meter_per_hour as volume rate units#406
iliekturtles merged 1 commit into
iliekturtles:masterfrom
Uzaaft:volume_rate

Conversation

@Uzaaft

@Uzaaft Uzaaft commented Feb 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR attempts to add cubic_meter_per_min and cubic_meter_per_hour as volume rate units.

@Uzaaft

Uzaaft commented Feb 4, 2023

Copy link
Copy Markdown
Contributor Author

Waiting on #405 before marking this as ready for review.a

@Uzaaft Uzaaft changed the title feat: Add ton_per_minute and ton_per_day as volume rate units feat(volume_rate): Add cubic_meter_per_min and cubic_meter_per_hour a… … eaaa8d4 …s volume rate units Feb 4, 2023
@Uzaaft Uzaaft changed the title feat(volume_rate): Add cubic_meter_per_min and cubic_meter_per_hour a… … eaaa8d4 …s volume rate units feat(volume_rate): Add cubic_meter_per_min and cubic_meter_per_hour as volume rate units Feb 4, 2023
iliekturtles
iliekturtles reviewed Feb 4, 2023
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! Changes look pretty good. Two minor changes below.

Comment thread src/si/volume_rate.rs Outdated
@Uzaaft

Uzaaft commented Feb 4, 2023

Copy link
Copy Markdown
Contributor Author

Thants for the suggestions, implemented those. :)
While I have you here @iliekturtles . I was wondering if you would be open to implement(or open for a contribution) that adds support for standard and normal conditions to the codebase?
https://en.wikipedia.org/wiki/Standard_temperature_and_pressure

@iliekturtles

iliekturtles commented Feb 6, 2023

Copy link
Copy Markdown
Owner

Similar to the other PR can you squash the commits and update the commit message. At the same time you can exclude the mass rate changes and just base this on top of iliekturles/master.

I'm definitely open to STP contributions. AmountOfSubstance already has a few STP specific units. Are you thinking about adding more or something different to more generally support STP?

@Uzaaft

Uzaaft commented Feb 11, 2023

Copy link
Copy Markdown
Contributor Author

Similar to the other PR can you squash the commits and update the commit message. At the same time you can exclude the mass rate changes and just base this on top of iliekturles/master.

I'm definitely open to STP contributions. AmountOfSubstance already has a few STP specific units. Are you thinking about adding more or something different to more generally support STP?

well, what I ideally would want is to have a general support of STP wherever it could apply, as well as adding methods to convert from/into other units. I'll have to take a look at how AmountOfSubstance does it.

@Uzaaft Uzaaft marked this pull request as ready for review February 11, 2023 17:16
@Uzaaft

Uzaaft commented Feb 11, 2023

Copy link
Copy Markdown
Contributor Author

Everything should be ready now.

@iliekturtles iliekturtles merged commit a7796b6 into iliekturtles:master Feb 11, 2023
@iliekturtles

iliekturtles commented Feb 11, 2023

Copy link
Copy Markdown
Owner

Thanks for the PR!

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.

2 participants

@Uzaaft @iliekturtles