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

isBinaryFileSync + isBinaryFile #11

Closed
thom4parisot opened this issue Mar 12, 2014 · 4 comments · Fixed by #12
Closed

isBinaryFileSync + isBinaryFile #11

thom4parisot opened this issue Mar 12, 2014 · 4 comments · Fixed by #12

Comments

@thom4parisot
Copy link

Reading the README, it is very strange to have isBinaryFileSync being async just because the second argument is a function.

Having the distinct functions would at least match Node.js sync/async declarations.

In any case, having a *Sync async function is just bad in term of naming.

@gjtorikian
Copy link
Owner

I think the README is just totally wrong. The async function is called isBinaryFile. The sync version is....also called isBinaryFile.

@thom4parisot
Copy link
Author

Thanks :-)

The thing is, considering the if/else thing, it would still be more implicit to have two different functions, while keeping consistant with Node fs module (readFile and readFileSync for example).

It is just a sugar but 1) the code gets more readable 2) smaller number of lines of code in the function and 3) easier to test.

@gjtorikian gjtorikian reopened this Mar 27, 2014
@sindresorhus
Copy link

👍 That is too magic.

What I usually do is export the async function on module.exports and then a sync function on module.exports.sync. Example: https://github.com/sindresorhus/read-chunk#api

@gjtorikian
Copy link
Owner

I'm going to implement a separate Sync method now, since there is interest.

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 a pull request may close this issue.

3 participants