doc: fix return results description of napi_get_value_string_utf8 #14529

taveras · 2017-07-28T15:37:50Z

The API for napi_get_value_string_utf8() appears to have been
previously changed. This improves the doc reflect the current design.

Fixes: #14398

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc, n-api

kfarnung · 2017-07-29T00:42:50Z

doc/api/n-api.md

-terminator. If the buffer size is insufficient, the string will be truncated
-including a null terminator.
+- `[out] result`: Number of bytes copied into the buffer, excluding the null
+terminator.


If the buffer size is insufficient, the string will be truncated including a null terminator.

I feel like this information should be represented somewhere, maybe as part of the bufsize parameter?

ah, yes! do you feel it may be best to keep this sentence within the [out] result description?

I think it would be better to move that sentence to be part of the [in] bufsize parameter comment. That's where it's relevant to call out that the string will be truncated, but still include a null terminator.

kfarnung

I think it's good to call out the possible truncation for inadequate buffer sizes. I don't think it belongs where it was though.

taveras · 2017-08-12T03:40:34Z

doc/api/n-api.md

- `[out] result`: Number of bytes copied into the buffer including the null.
-terminator. If the buffer size is insufficient, the string will be truncated
-including a null terminator.
+- `[in] bufsize`: Size of the destination buffer. When this value is insufficient, the returned string will be truncated.


@kfarnung I added the details about the string truncation, but now under the bufsize line

nit: needs a line wrap at 80 chars :-)

doh, pushed a fix 😄

mhdawson

LGTM

mhdawson · 2017-08-18T14:08:43Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/9736/

mhdawson · 2017-08-18T14:56:18Z

@taveras seems like there are now conflicts, can you rebase ?

The API for napi_get_value_string_utf8() appears to have been previously changed. This improves the doc reflect the current design. Fixes: #14398 PR-URL: #14529

taveras · 2017-08-18T17:12:20Z

@mhdawson i rebased the branch against master.

also, i added the same changes for the remaining napi_get_value_string_* methods,
as the original issue applied to the docs of all three methods

mhdawson · 2017-08-22T19:08:24Z

@taveras thanks :)

CI run: https://ci.nodejs.org/job/node-test-pull-request/9792/

mhdawson · 2017-08-22T21:34:22Z

CI failure was unrelated, opened #14982

mhdawson · 2017-08-22T21:34:37Z

Landing, sorry it took so long.

mhdawson · 2017-08-22T21:41:26Z

Landed as 82bad0b. @taveras Thanks !

The API for napi_get_value_string_utf8() appears to have been previously changed. This improves the doc reflect the current design. PR-URL: #14529 Fixes: #14398 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

The API for napi_get_value_string_utf8() appears to have been previously changed. This improves the doc reflect the current design. PR-URL: nodejs/node#14529 Fixes: nodejs/node#14398 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

The API for napi_get_value_string_utf8() appears to have been previously changed. This improves the doc reflect the current design. PR-URL: #14529 Fixes: #14398 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

The API for napi_get_value_string_utf8() appears to have been previously changed. This improves the doc reflect the current design. PR-URL: nodejs#14529 Fixes: nodejs#14398 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

The API for napi_get_value_string_utf8() appears to have been previously changed. This improves the doc reflect the current design. Backport-PR-URL: #19447 PR-URL: #14529 Fixes: #14398 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Jul 28, 2017

cjihrig approved these changes Jul 28, 2017

View reviewed changes

kfarnung reviewed Jul 29, 2017

View reviewed changes

kfarnung suggested changes Jul 29, 2017

View reviewed changes

jasnell approved these changes Jul 31, 2017

View reviewed changes

taveras commented Aug 12, 2017

View reviewed changes

kfarnung approved these changes Aug 12, 2017

View reviewed changes

jasnell approved these changes Aug 13, 2017

View reviewed changes

mhdawson approved these changes Aug 18, 2017

View reviewed changes

Daniel Taveras and others added 5 commits August 18, 2017 12:28

doc: fix return results description of napi_get_value_string_utf8

08d72e1

The API for napi_get_value_string_utf8() appears to have been previously changed. This improves the doc reflect the current design. Fixes: #14398 PR-URL: #14529

doc: add note regarding insufficient buffer size

1392292

doc: fix line to wrap at 80 chars

a337a5b

doc: remove unnecessary character

5b032a1

doc: fix results descriptions of napi_get_value_string_*

c607acb

jasnell approved these changes Aug 18, 2017

View reviewed changes

mhdawson closed this Aug 22, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: fix return results description of napi_get_value_string_utf8 #14529

doc: fix return results description of napi_get_value_string_utf8 #14529

taveras commented Jul 28, 2017

kfarnung Jul 29, 2017

taveras Jul 30, 2017

kfarnung Jul 30, 2017

kfarnung left a comment

taveras Aug 12, 2017

jasnell Aug 13, 2017

taveras Aug 16, 2017

mhdawson left a comment

mhdawson commented Aug 18, 2017 •

edited

Loading

mhdawson commented Aug 18, 2017

taveras commented Aug 18, 2017

mhdawson commented Aug 22, 2017

mhdawson commented Aug 22, 2017

mhdawson commented Aug 22, 2017

mhdawson commented Aug 22, 2017

doc: fix return results description of napi_get_value_string_utf8 #14529

doc: fix return results description of napi_get_value_string_utf8 #14529

Conversation

taveras commented Jul 28, 2017

Checklist

Affected core subsystem(s)

kfarnung Jul 29, 2017

Choose a reason for hiding this comment

taveras Jul 30, 2017

Choose a reason for hiding this comment

kfarnung Jul 30, 2017

Choose a reason for hiding this comment

kfarnung left a comment

Choose a reason for hiding this comment

taveras Aug 12, 2017

Choose a reason for hiding this comment

jasnell Aug 13, 2017

Choose a reason for hiding this comment

taveras Aug 16, 2017

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Aug 18, 2017 • edited Loading

mhdawson commented Aug 18, 2017

taveras commented Aug 18, 2017

mhdawson commented Aug 22, 2017

mhdawson commented Aug 22, 2017

mhdawson commented Aug 22, 2017

mhdawson commented Aug 22, 2017

mhdawson commented Aug 18, 2017 •

edited

Loading