search

probablykasper / cpc

Text calculator with support for units and conversion
https://crates.io/crates/cpc
MIT License
128 stars 14 forks source link

Support dividing lengthes by speed #17

Closed djmattyg007 closed 3 years ago

djmattyg007 commented 3 years ago

The result has a unit of time.

probablykasper commented 3 years ago

Awesome!

Maybe some more default units should be added, like if you divide a light year you might want a bigger time unit than hour

djmattyg007 commented 3 years ago

I think that would require a new speed unit, which would also require changes to the lexer (that I don't want to make until #18 is merged).

djmattyg007 commented 3 years ago

I updated the PR to provide more appropriate output when dealing with really small time units:

❯ target/release/cpc '100 km / 500 kph'
12.0 Minute

❯ target/release/cpc '10 km / 500 kph'
1.20 Minute

❯ target/release/cpc '1 km / 500 kph'
7.200 Second

❯ target/release/cpc '1 m / 500 kph'
7.200000 Millisecond

How would you feel about a follow-up patch that strips unnecessary decimal places?

probablykasper commented 3 years ago

Alright, let me clarify how the code works there, I should've explained earlier.

Inside divide(), there are two ways that you figure out the final unit:

  1. Based on the answer (how big is it?). This is what to_ideal_unit() does.
  2. Based on the input units. For example, if we divide a kilometer by an hour, we probably want the result to be kph. This is what we do for length/time, and we don't need to_ideal_unit() at all for that because all the units of speed are about the same weight.

If it fits, we could use a combination of both.

djmattyg007 commented 3 years ago

The other thing to consider is that it's super easy for users to request time values in a more suitable unit if they feel it's necessary:

❯ target/release/cpc '100 km / 0.000001 kph'
100000000 Hour

❯ target/release/cpc '100 km / 0.000001 kph in days'
4166666.666666666666666666666666667 Day

❯ target/release/cpc '100 km / 0.000001 kph in weeks'
595238.0952380952380952380952380952 Week

❯ target/release/cpc '100 km / 0.000001 kph in years'
11407.94586245211514724235724666945 Year
djmattyg007 commented 3 years ago

I'm in the middle of implementing data rate units, and had another thought on this subject. All data rates are typically measured in seconds, but if you were to perform this calculation:

70 megabytes divided by 20 kilobytes per second

You probably want an answer in minutes, not seconds. I'll need to expand the to_ideal_unit checks to support going the other way (converting large numbers of seconds and minutes to hours).

probablykasper commented 3 years ago

The other thing to consider is that it's super easy for users to request time values in a more suitable unit if they feel it's necessary:

Yeah, that's what you currently need to do if you for instance do m3/m and want a result a unit like Acre. Similarly, that's what we might want to have to do if we want months.

I'm in the middle of implementing data rate units, and had another thought on this subject. All data rates are typically measured in seconds, but if you were to perform this calculation:

70 megabytes divided by 20 kilobytes per second

You probably want an answer in minutes, not seconds. I'll need to expand the to_ideal_unit checks to support going the other way (converting large numbers of seconds and minutes to hours).

Ah, so that would be a 70mb file and you want to know how long it takes at 20kbs. With what I suggested, you would indeed end up with minutes in that case.

djmattyg007 commented 3 years ago

I've updated the PR. I think renaming the function should be left for another PR, and potentially a future version, as it would be a breaking change.

probablykasper commented 3 years ago

I think renaming the function should be left for another PR, and potentially a future version, as it would be a breaking change.

Yeah that might be fair. I've been pretty bad about introducing breaking changes in minor versions. That said, I think even adding new units would technically be a breaking change in Rust.

My original intent was actually for the internals just in case someone wanted to use them, but without them being considered stable. A better way to go about that would be to have an internals module, and to actually state it's not stable (or at least to have waited with 1.0).

djmattyg007 commented 3 years ago

Okay, I'm pretty sure I've made all the changes you've requested :pray:

probablykasper commented 3 years ago

Perfect! ❤️