tiffany352 / rink-rs

Unit conversion tool and library written in rust
https://rinkcalc.app/about
GNU General Public License v3.0
408 stars 28 forks source link

WIP: Provide access to result of previous calculation #107

Closed leftshift closed 2 years ago

leftshift commented 2 years ago

As discussed in #104, this adds access to the result of the previous operation via ans, ANS and _.

Was a bit harder than anticipated, as the results are converted to NumberParts before passed as a QueryReply, which doesn't contain the raw number result. As a workaround, I added the raw_number field to NumberParts, but it might make sense to instead convert to NumberParts at the very end instead?

To satisfy the derive(Serialize, Deserialize) on NumberParts, I had to make a few more things have these traits. I chose to enable the serde feature of the num crate and removed the custom serialize impl for BigRat. I'm not sure whether the NumberParts are actually serialized somewhere? And whether this change might break things there?

Also, this seems to have broken something with the unit canonicalisation:

> 10 ohm
10 ohm (resistance)
> _ * 100mA
1 ampereohm
> 10 ohm * 100mA
1 volt (electrical_potential)

Haven't quite tracked down where that goes wrong.

What do you think? I'd appreciate feedback on what I already did and how best to continue. Thanks!

leftshift commented 2 years ago

Also, right now, when there is no previous result, ans behaves like a unit that doesn't exist (because lookup returns None). I think that could be pretty confusing, but changing this would require either changing the parser or how lookup works/what it can return I think.

leftshift commented 2 years ago

Yay, thanks for the feedback!

Oops, did pretty much exactly those changes already but I forgot committing them it seems, thanks!

I think there still are some todos before merging:

I won't get around to working on that until in a few days probably.

leftshift commented 2 years ago

I think I understand the unit problem: The stored result isn't in base units, and by returning the display units from lookup like this, they incorrectly get treated as base units instead of being resolved back to base units. I'm not sure how this can be fixed, the cleanest way would probably be to try to also keep the Number in base units around when prettifying the result and store that?

leftshift commented 2 years ago

Okay, phew, got it, shouldn't have used the prettified value in to_parts. The units seem to work now, but I'm not 100% confident there isn't any other edge case where the units might go wrong. Turns out you can't really do pull requests for github wikis, but I guess I'll write something up and attach a patch to this discussion.

leftshift commented 2 years ago

Patch for the wiki: 0001-add-section-about-previous-result.txt

Feel free to reword or move to a different section, I wasn't really sure what's the best place for it in the manual.

tiffany352 commented 2 years ago

The patch to the wiki look good to me. Thanks!