Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support for Decimal within ta-rs. #59

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lukesneeringer
Copy link

@lukesneeringer lukesneeringer commented May 24, 2022

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
Copy link
Author

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

@lukesneeringer lukesneeringer marked this pull request as draft May 25, 2022 00:00
@lukesneeringer
Copy link
Author

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 lukesneeringer marked this pull request as ready for review May 31, 2022 19:40
@lukesneeringer
Copy link
Author

lukesneeringer commented May 31, 2022

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 lukesneeringer changed the title wip: Support for Decimal within ta-rs. feat: Support for Decimal within ta-rs. Jun 1, 2022
@lukesneeringer
Copy link
Author

@greyblake This is now ready! :-)

@lukesneeringer
Copy link
Author

@greyblake Ping! :-)

@greyblake
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Add support for rust_decimal.
2 participants