json doc generator now generates _index.json as it creates files. #3696

Closed
wants to merge 44 commits into
from

Conversation

Projects
None yet

iapain commented Jul 12, 2012

generate.js now accepts --output=<file> which tools/doc/json.js uses to write the output. If output file is missing then it executes callback. This was required to get the directory for new _index.json. Ideally, I left it for you to decide what to call this _index.json. There is already an index.json file which should not be generated at all. Additionally it sets title as first heading node text if applicable.

TODO

  • Add exclude files to avoid generation of index.json
  • Dependency map in Makefile (if subchapters like structure is required).
@iapain iapain Added output file option to gentest.
json doc generator now accepts output file as well.
json doc generator now generates index file automatically.
5152bf5

iapain closed this Jul 12, 2012

iapain reopened this Jul 12, 2012

iapain commented Jul 12, 2012

Damn I cannot attach it to the issue. It's for #3668

What's up with this pull request? Is it going to get merged?

iapain commented Jul 31, 2012

It still needs some API related decisions from @isaacs. He urged me to write this patch but probably too busy with other stuff.

I reckoned, just wanted to bump this a little.

isaacs commented Oct 3, 2012

The index should be written to index.json, not _index.json.

Also, it's a bit odd having url be a an object. Maybe it should be just html: 'foo.html', json: 'foo.json' or something?

iapain commented Oct 3, 2012

@isaacs The reason I kept _index.json was index.markdown translates into index.json but I guess it's no longer required. In last commit I have renamed it to index.json. Moreover url.html, url.json is now just html, json as you suggested.

Can one of the admins verify this patch?

Indeed, it would be awesome to get this in, I don't quite understand what's taking eight months to review ...

Owner

bnoordhuis commented Mar 14, 2013

Yeah, because we're all just twiddling our thumbs while raking in the big paychecks...

Look at the number of open issues and pull requests, now realize that there are only two or three people working on node.js full time.

If you want to do something constructive, start triaging bug reports and reviewing PRs.

@bnoordhuis I didn't say that you guys weren't doing something but eight months for such a minor PR?

I'm not actively working on Node.js, I have other projects I'm actively contributing to.

iapain commented Mar 14, 2013

I must agree with @mainerror being a contributor it's very discouraging. I tried to bring it up few times on IRC to @isaacs in vain.

@bnoordhuis I totally respect what you guys are doing it's pretty cool and cutting edge but eventually it's core team who needs to organize and expand if it's required.

@isaacs isaacs and 1 other commented on an outdated diff Mar 14, 2013

tools/doc/json.js
@@ -146,9 +159,62 @@ function doJSON(input, filename, cb) {
finishSection(current, stack[stack.length - 1]);
}
- return cb(null, root)
+ if (outfile) {
+ writeOututToFile(root, filename, outfile, indexfile, writeToIndexFile);
@isaacs

isaacs Mar 14, 2013

Typo? Should this be writeOutputToFile?

@iapain

iapain Mar 14, 2013

Sorry that's a typo. Will rectify in next commit.

@isaacs isaacs and 1 other commented on an outdated diff Mar 14, 2013

tools/doc/json.js
- return cb(null, root)
+ if (outfile) {
+ writeOututToFile(root, filename, outfile, indexfile, writeToIndexFile);
+ }
+ else {
+ return cb(null, root)
+ }
+}
+
+// write output object to outfile
+function writeOututToFile(obj, sourcefile, outfile, indexfile, cb) {
+ fs.writeFile(outfile, JSON.stringify(obj, null, 2), function(err) {
+ if(err) {
+ throw new Error('error saving file - '+ err);
+ }
+ cb(obj, sourcefile, path.join(path.dirname(outfile), indexfile));
@isaacs

isaacs Mar 14, 2013

API here is a bit weird. cb functions should pass err as their first argument. Better to just call fs.writeFile with the supplied cb, than to force a throw.

@iapain

iapain Mar 14, 2013

I tried to keep the way it worked with console.log version. I remember there it threw an error as well but I can see you're right and in this setup it doesn't make any sense to throw an error. I will clean up this in a moment.

isaacs commented Mar 14, 2013

In general, it is better to light candles than to curse darkness. @mainerror, if you have comments on this patch, then share them. We welcome reviews from all parties. Though it does take some core team member to actually land it, you can help things along by providing feedback.

@iapain I'm a bit confused about what happens if just one of the files changes. Ie, if you've generated the index.json, and then we change doc/api/url.markdown, for instance. Will it generate a new index.json containing all of the previous files as well? How does it know to do index.json last?

iapain commented Mar 14, 2013

@isaacs I'm not tracking anything, it just regenerates index.json. Should it keep track for changes?

iapain commented Mar 14, 2013

@isaacs changes checked-in. I think now I got it what you were trying to say in previous comment. Actually writeToIndexFile checks if there is already an index.json file. It also check (in a way) that if it's proper index.json and then it update or create proper chapter entry into main json object which is written back to index.json

Owner

Fishrock123 commented on 92598e8 Aug 7, 2014

o_o

+1

Member

rlidwka replied Aug 8, 2014

Awesome, thanks. :)

rofl. is 0.12 just being skipped? haha

Member

jbergstroem replied Aug 8, 2014

@defunctzombie even numbers are reserved for stable releases (not sure if irony, but chose to reply for possible others)

What's the next stable release then?

Member

jbergstroem replied Aug 9, 2014

@freeall The next stable is called 0.12. You can track its development here. I haven't seen any plans for a public release just yet, but assume it's coming.

Wow, awesome, 👍

tjfontaine and others added some commits Aug 7, 2014

@tjfontaine tjfontaine Merge remote-tracking branch 'upstream/v0.12' 912b5e0
@tjfontaine tjfontaine Merge remote-tracking branch 'upstream/v0.12' 7c04197
@cjihrig @trevnorris cjihrig net: remove use of arguments in Server constructor
The current implementation uses the arguments object in the Server()
constructor. Since both arguments to Server() are optional, there was a
high likelihood of accessing a non-existent element in arguments, which
carries a performance overhead. This commit replaces the arguments
object with named arguments.

Reviewed-by: Trevor Norris <trev.norris@gmail.com>
25702ab
@yorkie @chrisdickinson yorkie stream: remove duplicated expression
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-by: Chris Dickinson <christopher.s.dickinson@gmail.com>
cfcb1de

@chrisdickinson chrisdickinson commented on an outdated diff Nov 19, 2014

tools/doc/json.js
@@ -146,9 +159,64 @@ function doJSON(input, filename, cb) {
finishSection(current, stack[stack.length - 1]);
}
- return cb(null, root)
+ if (outfile) {
+ writeOutputToFile(root, filename, outfile, indexfile, writeToIndexFile);
@chrisdickinson

chrisdickinson Nov 19, 2014

Okay, I see the source of my confusion. writeToIndexFile performs async actions, but never calls cb. This probably needs to be something like the following:

writeOutputToFile(root, filename, outfile, indexfile, function(err, root, sourcefile, outfile) {
  if (err) return cb(err);
  writeToIndexFile(root, sourcefile, outfile, cb);
})

@chrisdickinson chrisdickinson commented on an outdated diff Nov 19, 2014

tools/doc/json.js
}
+// write output object to outfile
+function writeOutputToFile(obj, sourcefile, outfile, indexfile, cb) {
+ fs.writeFile(outfile, JSON.stringify(obj, null, 2), function(err) {
+ cb(err, obj, sourcefile, path.join(path.dirname(outfile), indexfile));
+ });
+}
+
+// make an entry into index file
+function writeToIndexFile(err, root, sourcefile, outfile) {
+ // check if there was an error writing file
+ if (err) {
+ throw new Error('error writing file - '+ e);
+ }
@chrisdickinson

chrisdickinson Nov 19, 2014

this error handling should happen in doJSON -- including the err argument that writeToIndexFile takes.

@chrisdickinson chrisdickinson commented on an outdated diff Nov 19, 2014

tools/doc/json.js
+ if (err) {
+ throw new Error('error writing file - '+ e);
+ }
+
+ // default type of an index
+ var obj = {"type":"index"};
+ var entry = {"source":sourcefile};
+
+ // check if indexfile already exists
+ if (fs.existsSync(outfile)) {
+ var data = fs.readFileSync(outfile);
+ try {
+ obj = JSON.parse(data.toString());
+ }
+ catch(e) {
+ throw new Error('invalid json data - '+ e);
@chrisdickinson

chrisdickinson Nov 19, 2014

If there's invalid JSON data, we should ignore it, and go with our default object.

@chrisdickinson chrisdickinson commented on an outdated diff Nov 19, 2014

tools/doc/json.js
+ var entry = {"source":sourcefile};
+
+ // check if indexfile already exists
+ if (fs.existsSync(outfile)) {
+ var data = fs.readFileSync(outfile);
+ try {
+ obj = JSON.parse(data.toString());
+ }
+ catch(e) {
+ throw new Error('invalid json data - '+ e);
+ }
+ }
+ // check if index file is valid
+ if (obj.type !== "index") {
+ throw new Error('invalid index file - '+ outfile);
+ }
@chrisdickinson

chrisdickinson Nov 19, 2014

I think this check is maybe unnecessary. Ultimately I think our goal should be to merge the chapters property into whatever's already there, or if nothing's there, create it.

@chrisdickinson chrisdickinson commented on an outdated diff Nov 19, 2014

tools/doc/json.js
+ }
+ // construct an entry object
+ entry.title = root.title;
+ entry.html = path.basename(sourcefile).replace(/\.(markdown|md)/i, ".html");
+ entry.json = entry.html.replace(/\.html/i, ".json");
+
+ // append mode
+ if (obj.chapters && typeof obj.chapters === "object") {
+ obj.chapters.push(entry);
+ }
+ else {
+ obj.chapters = [entry];
+ }
+ fs.writeFile(outfile, JSON.stringify(obj, null, 2), function(err) {
+ if(err) {
+ throw new Error('error saving file - '+ err);
@chrisdickinson

chrisdickinson Nov 19, 2014

return cb(new Error('[...]')) vs. throwing here.

@iapain Thanks for your patience. I went through and reviewed the changes and had a few comments, and will merge once they're addressed -- or, if you'd prefer I can take over this PR. Sorry you had to wait so long!

iapain commented Nov 20, 2014

@chrisdickinson Please take over this PR. Excited to see that it's finally coming :)

tjfontaine and others added some commits Jan 16, 2015

@tjfontaine tjfontaine Merge remote-tracking branch 'upstream/v0.12'
Conflicts:
	src/node_version.h
885f721
@frankcash @geek frankcash README: fix link text
Extends a hyperlink to cover the whole line

PR-URL: #8972
Reviewed-by: Colin Ihrig <cjihrig@gmail.com>
31d4847
@misterdjules misterdjules Merge remote-tracking branch 'upstream/v0.12'
Conflicts:
	src/node_version.h
d8baf8a
@misterdjules misterdjules Merge remote-tracking branch 'upstream/v0.12'
Conflicts:
	src/node_version.h
3917596
@misterdjules misterdjules src: update AUTHORS after merge of v0.12 in master b3fcc24
@bjouhier @cjihrig bjouhier fs: properly handle fd passed to truncate()
Currently, fs.truncate() silently fails when a file descriptor
is passed as the first argument. This commit changes this
behavior to properly call fs.ftruncate(). This commit also
adds proper type checking to the callback provided to
makeCallback().

PR-URL: nodejs#9161
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
4c31cda
@sam-github @misterdjules sam-github dgram: implicit binds should be exclusive
Server sockets should be shared by default, and client sockets should be
exclusive by default. For net/TCP, this is how it is, for dgram/UDP, its
a little less clear what a client socket is, but a socket that is
auto-bound during a dgram.send() is not usefully shared among cluster
workers, any more than an outgoing TCP connection would be usefully
shared.

Since implicit binds become exclusive, implicit/client dgram sockets can
now be used with cluster on Windows. Before, neither explicit nor
implicitly bound sockets could be used, causing dgram to be completely
unsupported with cluster on Windows. After this change, they become half
supported.

PR: nodejs#8643
Reviewed-by: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Bert Belder <bertbelder@gmail.com>
Reviewed-by: Julien Gilli <julien.gilli@joyent.com>
e42c4a3
@misterdjules misterdjules Merge remote-tracking branch 'upstream/v0.12' 4b69dcb
@misterdjules misterdjules src: enable strict mode in all builtin modules
This is a follow-up commit for b233131.

It enables strict mode in all built-in modules.

PR: #9302
PR-URL: nodejs#9302
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
ef43443
@misterdjules misterdjules Merge remote-tracking branch 'upstream/v0.12' a995a6a
@fastest963 @cjihrig fastest963 net: use cached peername to resolve remote fields
Allows socket.remote* properties to still be accessed even after the
socket is closed.

Fixes: nodejs#9287
PR-URL: nodejs#9366
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
8c38b07
@stcruy @cjihrig stcruy doc: fix '\\' typos on Windows
This commit changes the Windows examples in path.markdown to
correctly display '\\'.

PR-URL: nodejs#9412
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
2b64132
@misterdjules misterdjules Merge remote-tracking branch 'upstream/v0.12' ae58fc4
@amir-s @misterdjules amir-s url: resolve urls with . and ..
'.' and '..' are directory specs and resolving urls with or without the
hostname with '.' and '..' should add a trailing slash to the end of the
url.

Fixes #8992.

Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#9427
9b534e2
@misterdjules misterdjules Merge remote-tracking branch 'upstream/v0.12'
Conflicts:
	src/node_version.h
9010dd2
@misterdjules misterdjules Merge remote-tracking branch 'upstream/v0.12'
Conflicts:
	src/node_version.h
94beb29
@jasnell jasnell tls: more secure defaults
Port of io.js commit: nodejs/node@77f3586

Original commit message:

This updates the default cipher suite to an more secure list, which
prefers strong ciphers with Forward Secrecy. Additionally, it enables
`honorCipherOrder` by default.

Noteable effect of this change is that the insecure RC4 ciphers are
disabled and that Chrome negotiates a more secure ECDHE cipher.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#14383
72357e5
@jasnell jasnell tls: command-line switch and envar cipher-list override
Add command line switches and environment variables to override
the default cipher suite in tls.js

`--cipher-list` and `NODE_CIPHER_LIST` can be used to completely
override the default cipher list with a given value.

`--enable-legacy-cipher-list` and `NODE_LEGACY_CIPHER_LIST` can
be used to reset the default cipher list back to a known legacy
value shipped in prior Node.js releases

A new `getLegacyCiphers` method on the tis module allows
programmatic access to the old cipher list defaults.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#14383
1bf1546
@jasnell jasnell tls: make --enable-legacy-cipher-list less verbose
Based on commit feedback, make the PrintHelp for
--enable-legacy-cipher-list less verbose.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#14383
3f58ce6
@jasnell jasnell tls: pass in isolate with define string constant + style nits
Per the commit feedback, fix up style nits and pass in the
isolate with the NODE_DEFINE_STRING_CONSTANT macro.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#14383
3705736
@jasnell jasnell test: fixing a few nits in the test
typo and unnecessary options init

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#14383
4d9c81b

You've been pushing commits more often than I thought....

misterdjules and others added some commits May 14, 2015

@misterdjules misterdjules Merge remote-tracking branch 'upstream/v0.12'
Conflicts:
	src/node_version.h
0df5e1c
@mrkmg @misterdjules mrkmg src: fix ifdef comment
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25274
d13d7f7
Owner

jasnell commented Jun 1, 2015

@iapain @chrisdickinson ... I know it's been a while, but is this still something you want to pursue? If yes, the PR would need to be updated significantly. If not, we should likely go ahead and close.

jasnell and others added some commits May 19, 2015

@jasnell jasnell doc: improve http.request example
Fixes: nodejs#5317

Improve the example in the documentation to show
that response content can be chunked across multiple
`data` events.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25352
6f9b178
@devonharvey @jasnell devonharvey _http_server.js: fix typo in comment
Fix misspelling of 'response' on line 453

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25472
016ff77

iapain commented Jun 4, 2015

@jasnell I will implement @chrisdickinson suggestions in neat way. Hold on.

Owner

jasnell commented Jun 4, 2015

@iapain 👍

socketpair and others added some commits May 16, 2015

@socketpair @jasnell socketpair tls.createSecurePair(): fix documentation typo
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25341
4d3514f
@misterdjules misterdjules Merge remote-tracking branch 'upstream/v0.12'
Conflicts:
	doc/api/tls.markdown
	src/node_version.h
f9d783a
@hidekiy @jasnell hidekiy Fix improper sample code in http.markdown
You must consume the data from the response object. #8443

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25471
6671efa
@duzun @jasnell duzun fixed a typo: fs.ReadStream(filename) -> fs.createReadStream(filename)
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25453
8c262dd
@chrisneave @jasnell chrisneave docs: Fix grammar in Transform API text
The third sentence of the fifth paragraph of the documentation for
transform._transform() has several words omitted and makes no
sense. This fix fills in the missing words to clarify the passage.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25371
8140d10
@iapain @iapain iapain Added output file option to gentest.
json doc generator now accepts output file as well.
json doc generator now generates index file automatically.
63b3f9e
@iapain @iapain iapain Now json doc generator writes index.json file.\n Renamed url.html and…
… url.json to html and json
ab03aec
@iapain iapain Fixed some typos. `writeOutputToFile` does not raise and exception an…
…ymore.
428bea0
@iapain iapain Callback is properly propagated thanks to @chrisdickinson 02cfad5
@iapain iapain resolved conflicts 27b3909

iapain commented Jun 18, 2015

@jasnell Ready to review.

iapain commented Jun 18, 2015

@jasnell I've to close this PR and will send you another one shortly. Looks like git rebase messed up with previous commits.

iapain closed this Jun 18, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment