Skip to content

Fix: A few structural changes to the repository and added missing tests and fixed some failing tests.#325

Merged
siriak merged 20 commits into
TheAlgorithms:masterfrom
tjgurwara99:master
Sep 4, 2021
Merged

Fix: A few structural changes to the repository and added missing tests and fixed some failing tests.#325
siriak merged 20 commits into
TheAlgorithms:masterfrom
tjgurwara99:master

Conversation

@tjgurwara99

@tjgurwara99 tjgurwara99 commented Aug 28, 2021

Copy link
Copy Markdown
Member

Apologies for the big PR, I tried to make minimal changes but one change/fix lead to another broken thing so... here we are.

I have made a few changes to the repo and added RSA algorithm that was failing tests and had an incorrect implementation.

This is a big PR because I have changed some of the directory structure inside the repo. For example, it made more sense to call the modular exponentiation function as modular.Exponentiation than modular_arithmetic.ModularExponentiation which was done before (by me). Similar thing applies to cipher directories different ciphers.

I have also added a few algorithms that were needed for the RSA algorithm in the relevant directories, example modular.Inverse.

I also noticed that the sorting algorithms tests were not set up correctly and because of which every test passed even though the algorithms were incorrect, example QuickSort and RadixSort. Unfortunately however, because of the way I have fixed it, it has resulted in golangci-lint failing because of one of their open issue. I can fix it by changing the implementation but I think we should try and stick to go's ecosystem and use the tools provided by go team, that is golint. golint is much more stricter than golangci-lint in most cases (as far as I have seen and I think its a good thing). These tools are actually more strict and have a better overall effect on other go tools. But if the maintainers want me to fix it as it is, I will fix it - albeit the code will be a bit uglier.

Closes: #152
Closes: #303


The following are open issues that are actually fixed adding it here so that merge automatically closes these issues
Closes: #37
by #266
Closes: #269
by #324
Closes: #36
by #266

@siriak

siriak commented Aug 28, 2021

Copy link
Copy Markdown
Member

Let's switch to golint if it's stricter and more robust. Could you update the golang_lint_ant_test script in this PR?

@tjgurwara99

Copy link
Copy Markdown
Member Author

@siriak I looked into it and the go team have archived golint for some reason (about 4 months ago) so its highly unlikely that they will make progress in it. There are other tools that I can look at, would you like me to?

@tjgurwara99

tjgurwara99 commented Aug 28, 2021

Copy link
Copy Markdown
Member Author

this seems to be a good alternative. In the meantime, I've fixed the issue and everything should be fine now. Once PR #324 merges, I might have some conflicts but I can resolve them easily as I know what I have done here. Let me know if we should add staticcheck to our action?

@tjgurwara99 tjgurwara99 mentioned this pull request Aug 28, 2021
13 tasks
@tjgurwara99 tjgurwara99 requested a review from cclauss as a code owner August 28, 2021 13:23
@cclauss

cclauss commented Aug 28, 2021

Copy link
Copy Markdown
Member

https://pkg.go.dev/golang.org/x/lint is deprecated and frozen.

@tjgurwara99

tjgurwara99 commented Aug 28, 2021

Copy link
Copy Markdown
Member Author

https://pkg.go.dev/golang.org/x/lint is deprecated and frozen.

Yeah, and unfortunately there is no stand in replacement for it. The staticcheck github-action seems to be a good alternative but haven't used it personally so not sure.

@cclauss

cclauss commented Aug 28, 2021

Copy link
Copy Markdown
Member

My vote would be that we stick with https://github.com/golangci/golangci-lint which continues to be well maintained.

raklaptudirm
raklaptudirm previously approved these changes Aug 28, 2021
raklaptudirm
raklaptudirm previously approved these changes Aug 28, 2021
raklaptudirm
raklaptudirm previously approved these changes Aug 28, 2021
siriak
siriak previously approved these changes Aug 30, 2021

@siriak siriak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@siriak

siriak commented Sep 2, 2021

Copy link
Copy Markdown
Member

@tjgurwara99 could you resolve the conflicts so that I could merge?

@tjgurwara99 tjgurwara99 dismissed stale reviews from siriak and raklaptudirm via bb3a39f September 3, 2021 18:56

@siriak siriak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@siriak siriak merged commit afad8ff into TheAlgorithms:master Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants