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

Convert root to array if root is a string #152 #153

Merged
merged 2 commits into from
Apr 8, 2017
Merged

Convert root to array if root is a string #152 #153

merged 2 commits into from
Apr 8, 2017

Conversation

rohmanhm
Copy link
Contributor

@rohmanhm rohmanhm commented Apr 2, 2017

fix issue #152,

@codecov
Copy link

codecov bot commented Apr 2, 2017

Codecov Report

Merging #153 into master will not change coverage.
The diff coverage is 100%.

Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 566c777...38c11c7. Read the comment docs.

@tleunen
Copy link
Owner

tleunen commented Apr 3, 2017

Thank for the contribution @rohmanhm. Most users will probably only have one custom root directory, so supporting a string is nice.

cc @fatfisz I believe you also wanted the feature.

@fatfisz
Copy link
Contributor

fatfisz commented Apr 5, 2017

Hey, thanks for the contribution!

This is looking ok, though the tests could be improved a bit. The test that you've added does not actually involve resolving the path, so the only thing that's being tested is the lack of an error when trying to run reduce over a string.

My suggestion is to replace arrays in the whole test suite with a string root (I did that in my fork too), and add one test that shows that two roots are working (currently there is no test like that).

Would you be willing to do that @rohmanhm?

@rohmanhm
Copy link
Contributor Author

rohmanhm commented Apr 6, 2017

My suggestion is to replace arrays in the whole test suite with a string root

I'm done.

@fatfisz
Copy link
Contributor

fatfisz commented Apr 8, 2017

Ok, I think this can be merged!

@fatfisz fatfisz merged commit 2bcea0c into tleunen:master Apr 8, 2017
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.

None yet

3 participants