Addons doc fixes #1125
Addons doc fixes #1125
Conversation
|
LGTM |
|
I'd like to review this but don't have time today, please hold for me! |
|
Calling @rvagg. |
|
reviewing now .. I'm back in the saddle after a few weeks of intense work-related distraction |
|
technically LGTM but I'm concerned about the cognitive overhead introduced by the unnecessary Pinging some folks who may have relevant thoughts on communicating these kinds of things in an optimal way for their opinions: @kkoopa, @ceejbot, @wblankenship, @timoxley, @agnat, @springmeyer, @ralphtheninja, @No9 |
|
IMO the cognitive overhead of learning the V8 API far outweighs the load of seeing |
It's not a bad practice to show good ways of doing things though. I don't do any C++ and that stuff doesn't seem to add any cognitive overhead compared to the method declaration itself. |
|
Look at how much of the linecount of each of the examples is the namespace boilerplate: it dominates the first example. It is also not relevant to the points being discussed or exemplified in the source examples. My advice would be to spend the lines on something that only this documentation can do, not something better covered by v8's docs. If you like, add a brief section talking about v8 namespaces, with a pointer to the official v8 documentation. |
|
@ceejbot makes a good point. |
|
I disagree about not worrying too much about the code style. Most add-ons I look at seem to be copy/pasted from the examples and they are terrible because of it. I've had to file more than one pull request where someone stuck a Keep in mind that most people that write add-ons for io.js aren't C++ programmers. They're JS programmers trying their hand at C++ in order to get something working. They don't know what good C++ development practices are. |
|
If it's that important, write documentation about it in its own section. You can go into detail about why & how and write examples showing exactly what you recommend people do. Focus the examples on what matters for each section of documentation. |
|
Skipping boilerplate here and there is not going to be a problem for someone who knows it's boilerplate. Others will just copy everything verbatim either way. I agree with Ben, examples should not teach bad practices. |
|
Okay, seems most are in favor. I'll leave this open until tomorrow. If no one else speaks up, it's going in as-is. |
|
+1 for high coding standards in examples. On a side note: I'd use an anonymous namespace instead of namespace |
|
High coding standards are not antithetical to effective documentation, but sadly in this case a point that is apparently critically important is being left to be made by accident in the form of unexplained boilerplate. |
|
@agnat I named the namespace because some of the examples include header files. I can picture the confusion on a newbie's face when he includes the header in two source files and gets linker errors for one but not the other. |
|
@bnoordhuis, ah ok. My bad. I somehow had the impression they are all single file demos.
True. Unfortunately, concise examples are antithetical to ... uh... cut-and-paste-culture. I think we all agree that it would be a better read without the In any case I'm still +1 to land this... |
v8::Isolate::GetCurrent() is slated for deprecation. Replace its uses in the addons documentation with v8::Object::GetIsolate(), etc. PR-URL: nodejs#1125 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
Remove unnecessary v8::HandleScope uses from the addons documentation. C++ API callbacks run in an implicit v8::HandleScope, there is no need to declare one in the callback function. PR-URL: nodejs#1125 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
v8::Handle is on its way out, to be replaced with v8::Local. Update the addons documentation accordingly. PR-URL: nodejs#1125 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
Wholesale importing an entire namespace with `using namespace` is a bad practice. Remove it from the addons documentation and replace it with proper `using` directives. Wrap code in a namespace while we are here. PR-URL: nodejs#1125 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
It's good practice now to call JS functions that don't execute in a specific scope with v8::Null() as the receiver. Update the addons documentation. PR-URL: nodejs#1125 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
bnoordhuis
force-pushed
the
addons-doc-fixes
branch
from
fddafe4 to
99c79f8
Compare
R=@rvagg