Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addons doc fixes #1125

Merged
merged 5 commits into from
Mar 21, 2015
Merged

Addons doc fixes #1125

merged 5 commits into from
Mar 21, 2015

Conversation

bnoordhuis
Copy link
Member

R=@rvagg

@trevnorris
Copy link
Contributor

LGTM

@rvagg
Copy link
Member

rvagg commented Mar 13, 2015

I'd like to review this but don't have time today, please hold for me!

@bnoordhuis
Copy link
Member Author

Calling @rvagg.

@rvagg
Copy link
Member

rvagg commented Mar 18, 2015

reviewing now .. I'm back in the saddle after a few weeks of intense work-related distraction

@rvagg
Copy link
Member

rvagg commented Mar 18, 2015

technically LGTM but I'm concerned about the cognitive overhead introduced by the unnecessary namespace demo and also the explicit use statements for v8 namespaced objects. My vote would be for leaving using namespace v8; and letting the user decide what they want to use in practice and not using namespace demo for the same reasons. Our audience doesn't necessarily need to be told how to program C++ in an ideal way, just how to interact with core and V8.

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

@trevnorris
Copy link
Contributor

IMO the cognitive overhead of learning the V8 API far outweighs the load of seeing using v8::* at the top of the file. But I don't really care either way.

@Fishrock123
Copy link
Contributor

Our audience doesn't necessarily need to be told how to program C++ in an ideal way, just how to interact with core and V8.

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.

@ceejbot
Copy link

ceejbot commented Mar 18, 2015

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.

@Fishrock123
Copy link
Contributor

@ceejbot makes a good point.

@bnoordhuis
Copy link
Member Author

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 using namespace v8 in a header, which then broke after minor changes to v8.h.

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.

@ceejbot
Copy link

ceejbot commented Mar 18, 2015

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.

@kkoopa
Copy link

kkoopa commented Mar 19, 2015

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.

@bnoordhuis
Copy link
Member Author

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.

@agnat
Copy link
Contributor

agnat commented Mar 19, 2015

+1 for high coding standards in examples.

On a side note: I'd use an anonymous namespace instead of namespace demo. It is paste-proof and hardly any add-on provides C++ services. So they don't actually need a namespace. Just static linkage will do.

@ceejbot
Copy link

ceejbot commented Mar 19, 2015

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.

@bnoordhuis
Copy link
Member Author

@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.

@agnat
Copy link
Contributor

agnat commented Mar 19, 2015

@bnoordhuis, ah ok. My bad. I somehow had the impression they are all single file demos.

High coding standards are not antithetical to effective documentation [...]

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 using declarations. Not sure if this has been discussed elsewhere, but how about doing away with using ... altogether and just use fully qualified names in the examples?

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 <rod@vagg.org>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
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 <rod@vagg.org>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
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 <rod@vagg.org>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
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 <rod@vagg.org>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
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 <rod@vagg.org>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis bnoordhuis closed this Mar 21, 2015
@bnoordhuis bnoordhuis deleted the addons-doc-fixes branch March 21, 2015 13:07
@bnoordhuis bnoordhuis merged commit 99c79f8 into nodejs:v1.x Mar 21, 2015
@rvagg rvagg mentioned this pull request Mar 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants