search

sikanhe / reason-nodejs

Node bindings for Reason and Bucklescript
MIT License
100 stars 18 forks source link

Factor out file permissions flag type? #21

Closed yawaramin closed 4 years ago

yawaramin commented 4 years ago

In the file https://github.com/sikanhe/reason-node/blob/master/src/Fs.re , the file permissions flag is repeated for several bindings:

https://github.com/sikanhe/reason-node/blob/4e3243fa321373a8f023b5b9689e569a07b2bd5a/src/Fs.re#L56

https://github.com/sikanhe/reason-node/blob/4e3243fa321373a8f023b5b9689e569a07b2bd5a/src/Fs.re#L79

Etc.

We can factor out a flag type and inline the values, using the technique shown here: https://dev.to/yawaramin/inlined-values-in-bucklescript-247c

In fact the same thing can be done for the encoding parameter which is also repeated several times.

sikanhe commented 4 years ago

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

yawaramin commented 4 years ago

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

austindd commented 4 years ago

@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?