Fix issue #288#292
Conversation
Only extend the rem operation to use math.fmod if one or both of the two arguments is not an integer.
There was a problem hiding this comment.
This must be an and, not an or, otherwise some operations are not defined.
I think _quot could be included in this if-statement as well, because it also has a special case defined above.
There was a problem hiding this comment.
It actually doesn't though. See, it's checking for whether either is not an integer. That is functionally identical to checking whether it is not the case that both are integers. Unless there's something else I'm not thinking of.
There was a problem hiding this comment.
Ah yes, sorry.
Can you include _quot as well, though?
|
I'm fine with this patch, but I'd like a few tests around the results of this patch, just to verify the behavior is what we expect. I'll merge at that point. |
|
Yeah, clearly this issue is possible because no one is writing any numeric code with pixie yet. I found it while solving project Euler problem 1. |
|
I just added a few tests that I think handle all the possible cases, or at least most of them. All the tests are passing. Interesting fun fact: quot returns Integer, even with fed with two ratio arguments, because it is the nearest integer quotient. Very logical! |
|
Thanks @thomasmulvaney I fixed it to not use stringification. |
There was a problem hiding this comment.
Can you fix these asserts as well?
|
@heyLu Done. |
|
Looks great, we'll merge this as soon as the build finishes |
Only extend the rem operation to use math.fmod if one or both of the
two arguments is not an integer.
Fixes #288