-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Implemented sqrt for cmath #3047
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
Conversation
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.
Thank you for contributing! The added code looks great but I have a few comments.
- Please do not add
Cargo.lock
changes here. I have no idea why it is changed without anyCargo.toml
changes but I think this is not what you intended. - Please run rustfmt.
cargo fmt --all
is usually enough. - You may forgot to correctly set user.name and user.email. Search for
git config user.email
how to do it and revise your commit metadata bygit commit --amend --reset-author
I don't believe we're currently at the point where we can confidently test your code just by running the CPython test suite (see these lines of |
@fanninpm In this case, I think we'd better to edit the test file to run actual test rather than adding our own test. The running is suppressed but there is already tests |
a tip for new open source contributors: never use |
Fixed up the tests a bit to allow |
Hi @DimitrisJim , I'm about to raise a PR for acos and acosh implementation, were these comments in the test file suggested by you to @Codemonk-adi ? Should I wait for his PR to be merged? |
Yup. Lets wait and see how we'll be running the tests. I'm personally open to either having a new variable or doing it as I've currently done. Since I'll try and follow most PRs for @youknowone opinions here? |
My suggestion is replcing line 331 like below: complex_fn = getattr(cmath, fn, None)
if complex_fn is None:
continue Then newly added function always will be tested but no error will be raised for not added one. |
Yup, that was a better idea. (Ref #3039 to keep track of the PRs.) |
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.
Looks good to me, thanks!
Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com>
@Codemonk-adi |
@youknowone |
@Codemonk-adi You should also set |
This is my first time working with rust, pls tell me if my code is wrong, i ran the test but couldnt unserstand the output