Migrate to eventNameH-style API; update read functions
#49
Conversation
This was dropped in a previous commit
| -- | bufs <- Ref.read ref | ||
| -- | useStringCb $ Buffer.toString UTF8 $ Buffer.concat bufs | ||
| -- | ``` | ||
| dataH :: forall w. EventHandle (Readable w) (Buffer -> Effect Unit) (EffectFn1 Chunk Unit) |
There was a problem hiding this comment.
In general I'm not a big fan of single-character suffixes if we can use the word it indicates instead; is there a reason beyond saving characters to not use dataHandler here?
There was a problem hiding this comment.
No, there is not. But, I'm not willing to change that. The Discourse post requesting feedback was open for a while. While I originally used dataHandle, it was suggested to drop it. I received no objection to that. 8 days later, I made the v3.0.0 release, whose docs state that naming conventions should follow that.
There was a problem hiding this comment.
For a subjective reason, using the H suffix means the entire function and its callback can be put on one line. For example
emitter # on eventNameHSpecialVariant \arg1 arg arg3 arg4 ->
...
Instead of
emitter # on eventNameHandlerSpecialVariant
\arg1 arg arg3 arg4 ->
...
There was a problem hiding this comment.
For a subjective reason, using the H suffix means the entire function and its callback can be put on one line.
I don't see why the second example here has to be dropped to a new line.
No, there is not. But, I'm not willing to change that.
OK. Well, I don't have other feedback, so I suppose you're good to go — you don't strictly need an approval (there was no approval on the 3.0.0 release of node-event-emitter either, where these names were introduced).
There was a problem hiding this comment.
you don't strictly need an approval (there was no approval on the 3.0.0 release of node-event-emitter either, where these names were introduced).
I assumed I didn't need approval because I wrote the node-event-emitter library. 🤷.
As for these libraries, more people use them and a second pair of eyes will help ensure no new problems are introduced by my changes. But, if that's something people don't care about, then I suppose I could just update all of these libraries without any approval on the resulting PRs.
|
|
||
| -- | Listen for `data` events, returning data as a String. Note that this will fail | ||
| -- | if `setEncoding` has NOT been called on the stream. | ||
| dataHStr :: forall w. EventHandle (Readable w) (String -> Effect Unit) (EffectFn1 Chunk Unit) |
There was a problem hiding this comment.
Same question here, except regarding the string portion — other than the length of the name, is there a reason to prefer dataHStr instead of dataHandlerString? Personally, I much prefer the latter, especially because we ordinarily write out "string" in these libraries. See, for example, the String type or functions like writeString and readString defined in this module.
Description of the change
See changelog file for description.
Checklist: