Skip to content

Added Sum of Digits Implementation #684

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
merged 10 commits into from
Sep 13, 2021
Merged

Added Sum of Digits Implementation #684

merged 10 commits into from
Sep 13, 2021

Conversation

SpiderMath
Copy link
Contributor

The related question: https://the-algorithms.com/algorithm/sum-of-digits

There's no JavaScript implementation, so I implemented in JS. Also, I might have messed up writing the tests, since it was not exactly clear to me. So I'd request to please crosscheck the tests(, and if I have any mistake/error in there, I'll fix it)

@lgtm-com
Copy link

lgtm-com bot commented Sep 9, 2021

This pull request introduces 1 alert when merging d402327 into f04dec3 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@SpiderMath
Copy link
Contributor Author

Fixed the mistake

I intended to initially write a different implementation but I wrote something else 🤦‍♂️
@raklaptudirm
Copy link
Member

What happened to the package-lock.json?

@SpiderMath
Copy link
Contributor Author

Umm, I tried to install the Node Modules which were there for the project (you hadn't told me about Jest testing stuff on Discord yet) and I think the conflict might be because of that.

Also, I'll fix the 2 spaces problem, I thought setting tab space to 2 would suffice but looks like it didn't.

@raklaptudirm
Copy link
Member

For the styling problem, you need to style the code with standard.js.

@SpiderMath
Copy link
Contributor Author

Okay finally. Sorry I have nearly no experience with linters (I just have a eslintrc which use everywhere) so I kinda messed up...

@raklaptudirm
Copy link
Member

standard.js is easy to use with no configuration needed, so in my opinion it is the best for beginners, who are targeted by this repo.

@SpiderMath
Copy link
Contributor Author

I can't help but agree. But as a guy mostly used to ESLint and one who didn't try anything else, it was confusing.

Standard is really good, I don't doubt that. It was mostly my not-being used to it was what kinda messed them up. However, now that I know how it works, I will probably be able to contribute properly next time.

(Also can we merge the PR now? Looks like everything's fine now~)

@raklaptudirm
Copy link
Member

Could you remove the package-lock.json changes?

@SpiderMath
Copy link
Contributor Author

Umm, actually I don't exactly know how I could undo that 😅
Could you please tell me how to do that? ( Sorry if that sounds really dumb 😰 )

@raklaptudirm
Copy link
Member

Just run npm install and then commit it again.

@SpiderMath
Copy link
Contributor Author

Umm, but the thing which caused the changes in the package-lock was me running npm install
I still ran it again, and it generates the same Lockfile to the one I committed (by mistake).

Do you want me to copy the current package-lock from this Repo and put it into mine? Because that's the only other alternative I can think of at the moment.

@raklaptudirm
Copy link
Member

If npm install keeps the file the same, leave it.

@raklaptudirm raklaptudirm merged commit b3b4ad4 into TheAlgorithms:master Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants