Factor out file permissions flag type? about reason-nodejs HOT 3 CLOSED

I support the idea, esp for flags, its unclear if encoding are the same everywhere though.

from reason-nodejs.

@sikanhe gotcha. I'll send a PR for the flag parameter then.

from reason-nodejs.

@yawaramin @sikanhe , Hey, just so you guys know, I have another branch in which I am making dramatic changes to this module:
https://github.com/sikanhe/reason-node/tree/events-overhaul
To be honest, these changes are technically out of the scope of what I intended for the branch, but I noticed a lot of issues and inconsistencies with parts of the FS API. We just haven't touched it in a while...

As for the encoding parameter, I am removing it altogether across the library. Specifying an encoding in the function parameter will coerce the "data" parameter to string. That is, the expected type of data input will become string instead of Buffer.t, and for all readable streams, the output will be coerced to string as well. This is inherently unsafe, unless we want to do the abstract StringBuffer.t technique, and provide boxing/unboxing functions to the user. We have done this before, and I'm not sure it's worth it... I already mentioned this issue to @sikanhe before.

I personally think the best way to deal with this issue is to only allow Buffer.t, and eliminate the ability to pass an encoding argument. If the encoding does need to be specified, the Buffer API has a function to let you set the encoding of the underlying byte-array, which is 100% equivalent in practice. If users want to work with strings, they can use the Buffer.toString/Buffer.fromString functions, both of which provide a way to specify the encoding.

This is not a 100% perfect solution, but it will dramatically reduce the amount of hoops we have to jump through to make IO operations safe.

Anyway, those are some of the changes I want to make with my current branch, but I'd be happy to take a look at the solution you suggested @yawaramin. Maybe submit a PR to my branch and we can merge it upstream later?

from reason-nodejs.