doc: clarify node.js addons language#12898
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added addons 
sam-github left a comment
sam-github left a comment There was a problem hiding this comment.
Perhaps in the git commit message "language" should be "are C++"? Other than that, LGTM
@BethGriggs super nice of you 🍰 |
gibfahn commented May 9, 2017
@refack what specifically would you want adding? An actual sentence would be nice (although it could probably be left for a follow-on PR anyway). |
mhdawson commented May 9, 2017
I think the change should likely be made in the title as well (first line in the file), and anything that links to that title. |
doc/api/addons.md Outdated
There was a problem hiding this comment.
Good point @mhdawson, worth changing this as well.
I get these matches with a quick (rip)grep, @mhdawson do the n-api ones need changing?
➜ node git:(master) ❯ rg "C/C\+\+ Addons"~/wrk/com/node doc/api/_toc.md 10:* [C/C++ Addons](addons.html) 11:* [C/C++ Addons - N-API](n-api.html) doc/api/addons.md 1:# C/C++ Addons 234:section titled [C/C++ Addons - N-API](n-api.html). doc/api/n-api.md 15:outlined in the section titled [C/C++ Addons](addons.html).
There was a problem hiding this comment.
The N-API actually supports writing Addons in C so C/C++ is correct for them. The references to addons.html of course need to be updated everywhere.
refack commented May 10, 2017
@gibfahn@mhdawson what I ment before is that: is this change correct now that we have N-API? Maybe we should just remove the reference to a language? Also at line 8: -> |
I suspect that this doc should be updated to cover n-api, but that's probably something that can/should be done separately. This change makes sense as-is. Also if we talk about |
refack commented May 10, 2017
probably, but FWIW both nan and N-API are covered lower in the doc. |
mhdawson commented May 10, 2017
@refack I had the same discussion with @sam-github and he convinced me this change was ok as existing addons are C++ only and the original section only dealt with them. The nan and later N-API sections were inserted, but the overall flow text not updated well enough to integrate them nicely. What seemed to make sense was to let this change go in now, as it is valid to the section to which it applies, and then plan a more complete update once N-API is no longer experimental. |
refack commented May 10, 2017
Sound good. |
gibfahn commented May 11, 2017
@mhdawson LGTY? |
gibfahn commented May 11, 2017
sam-github commented May 11, 2017
Landed in abfd4bf |
PR-URL: #12898Fixes: #7129 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#12898Fixes: nodejs#7129 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
MylesBorins commented Jun 22, 2017
This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported |
PR-URL: nodejs#12898Fixes: nodejs#7129 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Backport-PR-URL: #19447 PR-URL: #12898Fixes: #7129 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
14 participants
Fixes: #7129
Checklist
Affected core subsystem(s)
doc