-
Notifications
You must be signed in to change notification settings - Fork 130
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
base: master
Are you sure you want to change the base?
Conversation
Note: I'm by no means done, I wanted to get feedback from you before doing the grunt work. :-) |
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 |
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 EDIT: It turns out the rand implementation was partial; I've sent paupino/rust-decimal#519 to provide what the benchmark needs.
|
@greyblake This is now ready! :-) |
@greyblake Ping! :-) |
@lukesneeringer Pong. Sorry for delays, I barely have time these days because of personal reasons. |
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:And then it's mostly a matter of taking the
f64
s in the various indicators and changing them toNumberType
.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
implementsInto<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.