Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add Fermat Primality Test #790

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

Merged
raklaptudirm merged 5 commits into TheAlgorithms:master from arthurvergacas:Fermat-Primality-Test
Oct 20, 2021
Merged

Add Fermat Primality Test #790

raklaptudirm merged 5 commits into TheAlgorithms:master from arthurvergacas:Fermat-Primality-Test
Oct 20, 2021

Conversation

Copy link
Contributor

@arthurvergacas arthurvergacas commented Oct 14, 2021

Hello!

This is my contribution to the math projects! It's a probabilistic primality test based on Fermat's little theorem.

If there's anything wrong with my pull request, please don't hesitate to comment. I've been struggling a lot with git, since this is one of my first pull requests.

Thank you!

defaude and others added 2 commits October 14, 2021 22:32
It seems you've accidentally swapped the implementation and the test file :)
The overall comment describing the algorithm (VERY nice doc, by the way) is not "proper" JSdoc => only one leading asterisk. It's generally considered good style to start a comment block (both JSdoc and regular comments) with a single, short sentence.
Further down, there were some git hiccups, most likely caused by merge conflicts?
Copy link
Contributor

defaude commented Oct 14, 2021

Hi there 👋

I have a few suggestions and I'd love to push them directly to the branch in your repo but I don't have access there. Could you add me to your fork?

https://github.com/arthurvergacas/Javascript/settings/access > "Add people"

Copy link
Contributor Author

Hi 👋 😄
Of course I could! And in fact, if nothing went wrong, I think that I've invited you. Please check wherever we check when we are invited to a repo (hahasd I really don't know much about it yet).

Copy link
Contributor

defaude commented Oct 15, 2021

All good, thank you! You can take a look at the changes I've done in arthurvergacas@c5e44d4

Copy link
Contributor Author

Hey @defaude, just wanted to say thank you more time! You helped me a lot, and I am really grateful for that! :) I've commented on the commit, but it's just me thanking you one more time haha. I suppose everything is fine with the pr now. Thank you! :))

@raklaptudirm raklaptudirm added algorithm Adds or improves an algorithm changes required This pull request needs changes feature Adds a new feature Reviewed labels Oct 15, 2021
- Add default number of iterations
- Correct "corner cases" to "edge cases"
- Make clear which bounds are inclusive and which are exclusive
Copy link
Contributor Author

Hello! I changed all the things that you pointed out, and also changed a little the test cases (now it also tests different numbers of iterations, which makes sense and should be there since the beginning - don't know why I didn't do that in the first place).

Hope that everything is fine now! But if there is still something to change, don't hesitate to ask! :)

- Add some explanation and links about Carmichael Numbers
- Remove explanation about in-built function Math.random()
Copy link
Contributor Author

I forgot to add in the commit message, but I also added some line breaks to the introductory explanation to made it easier to read :)

@raklaptudirm raklaptudirm removed the changes required This pull request needs changes label Oct 20, 2021
@raklaptudirm raklaptudirm merged commit a99ba4f into TheAlgorithms:master Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raklaptudirm raklaptudirm raklaptudirm approved these changes

Assignees
No one assigned
Labels
algorithm Adds or improves an algorithm feature Adds a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /