Fraction numerator denominator helpers#3605
Conversation
josdejong
left a comment
There was a problem hiding this comment.
Thanks for working out these functions num and den @AnslemHack . Your PR looks well taken care of!
I made a few inline comments, can you have a look at those?
On a side note: the detailed description of your PR looks AI-generated and doesn't really add useful information to me 😅. I think it's enough to explain that "this PR addresses #3595, implementing two new functions, num and den".
my apologise for the verbose @josdejong , I must have over refined my output description, I have now made changes to that with a brief description of what it simply does, hope this is better |
|
Thanks! |
|
A few more items:
|
…nslemHack/mathjs into fraction-numerator-denominator-helpers
thanks for your feedback I have now addressed your concerns. please take another look. |
gwhitney
left a comment
There was a problem hiding this comment.
Thanks for great progress! Just a few things were overlooked or need to be brought into consistency with the rest of mathjs. Thanks!
|
@gwhitney @josdejong I believe to have addressed all concerns on this pr now.. would appreciate if you can have a look, still open for any further feedbacks or adjustment that might be required |
|
Yes, very close now. Just a couple of open items, and please in addition to taking care of those also update to be on top of the latest version of the |
@gwhitney @josdejong I have addressed most concerns now, mind taking another look in this pr? |
gwhitney
left a comment
There was a problem hiding this comment.
The new functions appear to comply with all of the provisions for adding a new function and to work as desired. Merging.
This PR fixes #3595 introduces two new arithmetic functions, num() and den(), which extract the numerator and denominator from Fraction objects respectively.