Skip to content

Conversation

erizocosmico
Copy link

Hi there! I've implemented a very early stage writeFile and writeFileSync. There's a few issues right now that I'd like to get some feedback on

  • for a more node-like API, data should either be string or bytes. I saw that write used only bytes, so I went with that for now. Should I add both, string and bytes or leave it just with bytes? If we decide to go with bytes only encoding would not be needed (which is why it's not being used right now).
  • i'm ignoring the result of write, since node api does not give anything other than the error. Should I return it?

Tests are missing yet

@kennetpostigo
Copy link
Owner

kennetpostigo commented Jan 8, 2018

@erizocosmico Thank you for this! So right now write uses bytes for buffer but, but I'm not sure that is right to use, or if another type should be used, aside from writeFile, there is also writeString, so It might be best to use bytes if that is correct of a buffer type.

I wouldn't worry about the callback return.

@kennetpostigo
Copy link
Owner

kennetpostigo commented Jan 8, 2018

@erizocosmico instead of an encoding object we could just use a encoding string instead, in the node.js docs it says you could use either.

@kennetpostigo
Copy link
Owner

Hey @erizocosmico , The project has been renamed to lwt-node so there have been quite a few changes. Sorry for the trouble, do you think you can sync this PR back up with master?

@erizocosmico
Copy link
Author

@kennetpostigo done! So, if we're going with bytes, should we let the user encode the string before saving the byte chunk instead of handling the conversion to string and the encoding in this functions?

@kennetpostigo
Copy link
Owner

@erizocosmico I think we should make the user encode the string before saving the byte chunk that way it's explicit of what is going on, do you agree?

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.

2 participants