Skip to content
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

Major refactor #6

Closed
wants to merge 8 commits into from
Closed

Conversation

MarshallAsch
Copy link

Hello @Nuno135.

I was using this as a dependancy for one of my projects and decided to look at the code.

I noticed that there were 2 implementations of meme one that used callbacks and one that used promises. The main part of this refactor was to combine it into one implementation. That would use a callback if one was given and otherwise use a promise.

This version also split the implementation into several smaller functions so that they can be tested and some of the logic can be decoupled.

I also added some unit tests for both the untils.js and the index.js files. (it currently does not have 100% coverage because I did not mock the web request library).

I also noticed that the package.json file was broken when I went to run npm install to I proposed a fix for that too.

Github shows that there are something like 13,000 additions in this PR but that is because of the addition of the package-lock.json file. (Which is considered to be a best practice).

Let me know if there is anything else that you would like done as part of this PR.

@MarshallAsch MarshallAsch mentioned this pull request Feb 24, 2021
@imposd
Copy link
Collaborator

imposd commented Mar 8, 2021

Sorry for the late reply. We did see your pull-request about 2 months ago. But things got hectic, and we lost track of time. Although we have an upcoming rewrite in TypeScript soon, so we won't be accepting your pull request. But we have credited your efforts in our readme for everyone to see! Linking to this pull-request. So your hard work will not be put to waste. Thank you for taking the time to tidy up things.

@imposd imposd closed this Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants