-
Notifications
You must be signed in to change notification settings - Fork 7.3k
buffer: reword Buffer.concat error message #8723
Conversation
this brings the error messaging in line with other node TypeError messages. fixes nodejs#7766.
LGTM |
👍 |
@chrisdickinson was this merged? |
@trevnorris It hasn't been merged. Technically it could be considered as an API change, but it's very unlikely that anyone relies on this error's message if/when catching it. @joyent/node-coreteam Is anyone opposed to landing it in v0.12? If not, I'll land it in v0.12 and master, otherwise just in master. Thank you! |
I'm fine landing this on v0.12. |
LGTM. Note, #9174 will touch this also. |
this brings the error messaging in line with other node TypeError messages. fixes nodejs#7766. PR: nodejs#8723 PR-URL: nodejs#8723 Reviewed-by: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@users.noreply.github.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This actually also affects v0.10, so we could land it there first. @joyent/node-coreteam Thoughts? |
No reason it couldn't go in 0.10. Just not sure if it's worth the time to do so. |
@cjihrig Considering there are far more users of v0.10.x than users of v0.12.x, I think it's worth the time to land in v0.10. Especially since it's quick to "backport" to v0.10 and then to merge in v0.12 and master. |
@joyent/node-coreteam Adding to the 0.10.37 milestone and will land in v0.10 asap as it's not an API change nor a breaking behavior change. |
this brings the error messaging in line with other node TypeError messages. fixes nodejs#7766. PR: nodejs#8723 PR-URL: nodejs#8723 Reviewed-By: James M Snell <jasnell@users.noreply.github.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Landed in d01a900, and merged in v0.12 and master with b0425f0 and a995a6a. Thank you @chrisdickinson! |
@chrisdickinson Are you going to PR this to iojs too? |
@piscisaureus taken care of. |
this brings the error messaging in line with other node TypeError messages.
fixes #7766.