Skip to content

Conversation

arthurvergacas
Copy link
Contributor

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?
@defaude
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"

@arthurvergacas
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).

@defaude
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

@arthurvergacas
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
@arthurvergacas
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()
@arthurvergacas
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
Labels
algorithm Adds or improves an algorithm feature Adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants