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

Adding promise support using universalify #109

Merged
merged 7 commits into from
Sep 8, 2018

Conversation

kartik2406
Copy link
Contributor

I saw your comment on this pull request, #103.
I used universalify to add promisesupport. Must admit the solution looks very clean this way.
Could you please review this?

Things changed:

  1. Added universalify to package.json
  2. Imported universalify in index and created promise based functions for read and write File
  3. ran npm test, got standar erros
  4. ran standard --fix, hence the reformatting

Could you tell me if i need to change something or if the changes look good to you.

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 5, 2018

Please remove the package-lock.json.

@kartik2406
Copy link
Contributor Author

Done 😄

@kartik2406
Copy link
Contributor Author

Hey i also added documentation in read-me. Could you please check that out too?

README.md Outdated Show resolved Hide resolved
@RyanZim
Copy link
Collaborator

RyanZim commented Sep 6, 2018

Also should add tests for promise usage.

@kartik2406
Copy link
Contributor Author

@RyanZim sure i will add tests. I had one doubt though.
For few tests, one example "> when invalid JSON and throws set to false"
there have been two calls made to the function, one with and one without options argument. As per my understanding we don't need to check without the options lag in this case right? or am i missing something here?

@kartik2406
Copy link
Contributor Author

I have added tests for promises, also have refactored existing tests so that common code can be shared among callback and promise tests (using beforeEach)

test/read-file.test.js Outdated Show resolved Hide resolved
test/read-file.test.js Outdated Show resolved Hide resolved
test/read-file.test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

LGTM

@RyanZim RyanZim merged commit 709c99a into jprichardson:master Sep 8, 2018
@RyanZim RyanZim mentioned this pull request Sep 8, 2018
@RyanZim
Copy link
Collaborator

RyanZim commented Sep 8, 2018

Released in 5.0.0 🎉

@kartik2406
Copy link
Contributor Author

yaay 😄 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants