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

Promises support #8

Merged
merged 16 commits into from Mar 4, 2019

Conversation

Projects
None yet
2 participants
@piranna
Copy link
Collaborator

piranna commented Feb 18, 2019

Add support for fs.promises. It also add support for the newer functions not available before. The pull-request is not bacward compatible due to removal of SyncWriteStream function (seems a typo for me since I was not able to find it in the docs, please confirm and I'll re-add it if needed), and fix resolution of up dir to work the same way as .. direntry in root dir, that in POSIX systems it points to itself, so /../blah is the same as /blah.

Please tell me any concerns do you have :-)

Show resolved Hide resolved index.js Outdated
@piranna

This comment has been minimized.

Copy link
Collaborator Author

piranna commented Feb 18, 2019

There was still some conflicts, now they are fixed.

@juliangruber

This comment has been minimized.

Copy link
Owner

juliangruber commented Feb 18, 2019

This looks great, thank you! Would you mind also adding some tests, so we can verify this doesn't break in the future? If not that is ok also and I can merge as is.

@piranna

This comment has been minimized.

Copy link
Collaborator Author

piranna commented Feb 18, 2019

This looks great, thank you!

You are welcome :-)

Would you mind also adding some tests, so we can verify this doesn't break in the future? If not that is ok also and I can merge as is.

No problem. Can you be able to configure Travis CI and Coveralls while I'm adding them? :-)

@juliangruber

This comment has been minimized.

Copy link
Owner

juliangruber commented Feb 18, 2019

Travis is set up already. Coveralls I've never done, if I add you as an owner on this repo, could you do it? And would you be interested in maintaining this in general?

@piranna

This comment has been minimized.

Copy link
Collaborator Author

piranna commented Feb 18, 2019

Travis is set up already

But... it's not checking the pull-requests :-/ Maybe you need to enable that feature?

Coveralls I've never done, if I add you as an owner on this repo, could you do it?

Yes, I think there would not be problems.

And would you be interested in maintaining this in general?

Maybe :-) Main problem is publishing to npm, besides that I would not have so much trouble having another child to feed :-D

@juliangruber

This comment has been minimized.

Copy link
Owner

juliangruber commented Feb 18, 2019

But... it's not checking the pull-requests :-/ Maybe you need to enable that feature?

It should run on the next commit, travis was disabled for some reason.

Yes, I think there would not be problems.

Invited you :)

Maybe :-) Main problem is publishing to npm, besides that I would not have so much trouble having another child to feed :-D

I can add you to both!

@piranna

This comment has been minimized.

Copy link
Collaborator Author

piranna commented Feb 18, 2019

But... it's not checking the pull-requests :-/ Maybe you need to enable that feature?

It should run on the next commit, travis was disabled for some reason.

Nice.

Yes, I think there would not be problems.

Invited you :)

Thank you! :-)

Maybe :-) Main problem is publishing to npm, besides that I would not have so much trouble having another child to feed :-D

I can add you to both!

That would be cool :-)

By the way, I have just enabled Coveralls for the repo, just only last to generate coverage info and send it.

@piranna

This comment has been minimized.

Copy link
Collaborator Author

piranna commented Feb 19, 2019

I have updated the code to use ES8 features and optimize the resolve algorythm. I've also changed the API to make the filesystem optional using the Node.jsfs module as default value, and added a _resolve() function bound to the used base dir.

Note: tests are not passing because you've configured Travis to use Node.js 0.10, not Node.js 10 :-P

@juliangruber

This comment has been minimized.

Copy link
Owner

juliangruber commented Feb 20, 2019

Note: tests are not passing because you've configured Travis to use Node.js 0.10, not Node.js 10 :-P

Just change the travis.yml, we'll release a major after this

@piranna

This comment has been minimized.

Copy link
Collaborator Author

piranna commented Feb 20, 2019

Done, tests are passing. Should we update the tests to use jest so we can have code coverage by free, or is it good enough just like how tests are at this moment?

@juliangruber

This comment has been minimized.

Copy link
Owner

juliangruber commented Feb 20, 2019

do as you wish :) either is ok for me

@piranna

This comment has been minimized.

Copy link
Collaborator Author

piranna commented Mar 1, 2019

Tests converted to Jest :-)

@piranna piranna requested a review from juliangruber Mar 1, 2019

@juliangruber
Copy link
Owner

juliangruber left a comment

nice work!

@piranna

This comment has been minimized.

Copy link
Collaborator Author

piranna commented Mar 4, 2019

nice work!

Thank you! :-D

@piranna piranna merged commit aa51ae4 into juliangruber:master Mar 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.