ta-rs icon indicating copy to clipboard operation
ta-rs copied to clipboard

feat: Support for Decimal within ta-rs.

Open lukesneeringer opened this issue 2 years ago • 6 comments

Okay @greyblake, here's how I have in mind approaching adding Decimal support for those that want it.

Essentially the idea is to add an optional dependency on rust_decimal, and swap the type based on whether or not the feature is added. I added this:

#[cfg(not(feature = "rust_decimal"))]
pub(crate) type NumberType = f64;
#[cfg(feature = "rust_decimal")]
pub(crate) type NumberType = rust_decimal::Decimal;

And then it's mostly a matter of taking the f64s in the various indicators and changing them to NumberType.

I'll probably need to do some work to get the tests working in both cases, since you (quite reasonably) use literals in a lot of tests. I'm not sure off hand how many types rust_decimal implements Into<Decimal> for, but worst case I can write a macro to make the tests do the right thing without a lot of repetition.

Fixes #55.

lukesneeringer avatar May 24 '22 23:05 lukesneeringer

Note: I'm by no means done, I wanted to get feedback from you before doing the grunt work. :-)

lukesneeringer avatar May 25 '22 00:05 lukesneeringer

Okay, I've gotten farther on this. The concept works and seems to scale. I've converted all the indicators and am about 2/3 of the way through the tests.

The major downside is literals in tests. Integers generally do the right thing but floats don't, so I had to write a lit! (literal) macro and use it on literals. This adds a trivial tax on writing indicators (there are about 7-8 literals total) but a noticeable tax on writing tests, which use literals liberally. I still think this is a worthwhile endeavor and hope you'll accept it, but wanted to be transparent.

lukesneeringer avatar May 26 '22 21:05 lukesneeringer

Okay @greyblake, this is done (with one caveat) and ready for review. I've detailed the overall approach above.

One thing that's not done is the benchmark, which requires rust_decimal to support rand. As it happens, they've just added that (three days ago; I did not do it), so ~I'm just waiting for a 1.24 release (which I have also politely inquired about).~

EDIT: It turns out the rand implementation was partial; I've sent paupino/rust-decimal#519 to provide what the benchmark needs.

  • Tests all pass both with and without the decimal feature.
  • Examples only pass without decimal, and I've configured CI accordingly. It did not seem proper to me to clutter up your documentation / examples.

lukesneeringer avatar May 31 '22 19:05 lukesneeringer

@greyblake This is now ready! :-)

lukesneeringer avatar Jun 17 '22 19:06 lukesneeringer

@greyblake Ping! :-)

lukesneeringer avatar Jun 30 '22 21:06 lukesneeringer

@lukesneeringer Pong. Sorry for delays, I barely have time these days because of personal reasons.

greyblake avatar Jul 01 '22 07:07 greyblake