-
-
Notifications
You must be signed in to change notification settings - Fork 418
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
Add LeastCommonMultiple and GreatestCommonDivisor to math package #4001
Conversation
Co-authored-by: Joe Eli McIlvain <[email protected]>
Co-authored-by: Joe Eli McIlvain <[email protected]>
@ponylang/committer this is ready for another review. I've address @jemc's comments. |
@EpicEric are you good with this now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect some tests for the new functionality. Other than that, I left a minor style suggestion.
What do you expect as tests? I don't see much value in a couple of unit tests. I could see some value in property tests if ponycheck becomes part of the standard library. If you have an idea for tests, please feel free to add. |
I'd also prefer property tests, but since these aren't in the stdlib yet, I personally feel that simple tests are better than no tests. Some simple tests could be:
t.assert_eq[U32](GreatestCommonDivisor[U32](373, 1231)?, U32(1))
t.assert_eq[U32](LeastCommonMultiple[U32](373, 1231)?, U32(373) * U32(1231))
t.assert_eq[U32](GreatestCommonDivisor[U32](0x506F6E79, 0x506F6E79)?, U32(0x506F6E79))
t.assert_eq[U32](LeastCommonMultiple[U32](0x506F6E79, 0x506F6E79)?, U32(0x506F6E79)) |
@EpicEric tests added |
Adds code for determining least common multiple and greatest common divisor to the standard library math package.