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

doc: link and style parameter types #4741

Conversation

claudiorodriguez
Copy link
Contributor

Changes the parsing of parameter types, links to MDN or
nodejs docs. See #4350

Replaces PR #4378

@claudiorodriguez
Copy link
Contributor Author

@jasnell Remade the PR fresh off master to make sure everything is clean, since this touches a bunch of docs the merging may produce some conflicts for other open PRs, but they should be minor

@r-52 r-52 added the doc Issues and PRs related to the documentations. label Jan 18, 2016
@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

LGTM
@nodejs/documentation

@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

Thank you @fansworld-claudio!

@cjihrig
Copy link
Contributor

cjihrig commented Jan 18, 2016

LGTM, but I'll defer to the docs team.

@@ -666,8 +666,8 @@ The `private_key` argument can be an object or a string. If `private_key` is a
string, it is treated as a raw key with no passphrase. If `private_key` is an
object, it is interpreted as a hash containing two properties:

* `key` : A string holding the PEM encoded private key
* `passphrase` : A string of passphrase for the private key
* `key` : {String} - PEM encoded private key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other places didn't have colons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it would be better if the type and description seperator is consistent in all the files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thefourtheye I agree, but unfortunately everything was already inconsistent. I think refactoring that belongs to another PR IMHO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there are quite a few areas here where incremental improvements need to be made. We'll get there but breaking it up into smaller incremental changes is likely best

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell Yup, I completely agree. That would be much easier to review as well :)

@thefourtheye
Copy link
Contributor

LGTM and Thanks a lot @fansworld-claudio :-)

@@ -641,7 +641,7 @@ See `waitpid(2)`.
### Event: 'message'

* `message` {Object} a parsed JSON object or primitive value.
* `sendHandle` {Handle object} a [`net.Socket`][] or [`net.Server`][] object, or
* `sendHandle` {Handle} a [`net.Socket`][] or [`net.Server`][] object, or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link does not appear to work.

@Fishrock123
Copy link
Contributor

I like this. In any case it's a step forward.

];
const typeMap = {
'Buffer': 'buffer.html#buffer_class_buffer',
'Handle': 'net.html#net_server_listen_handle_callback',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle probably needs to be more specific, there are many types of handles, although they all inherit from AsyncWrap afaik.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123 I'm a bit out of my depth there. I'm fixing the link, but no idea where to link to exactly other than that.

@chrisdickinson
Copy link
Contributor

This is excellent work! LGTM.

@thefourtheye
Copy link
Contributor

I think it would be better if the tools part and the doc part are in different commits.

@claudiorodriguez
Copy link
Contributor Author

Corrected a couple of things noted, thanks everyone. @thefourtheye you mean two commits in a single PR, or two separate PRs? I could do any of the two, if the current contents of the PR are okay

@thefourtheye
Copy link
Contributor

@fansworld-claudio Given the fact that it has been reviewed by quite a few collaborators and owners, I would say splitting into two commits in this PR itself would be better.

@claudiorodriguez
Copy link
Contributor Author

@thefourtheye is something like this alright?

@thefourtheye
Copy link
Contributor

@fansworld-claudio Yup! :-)

@silverwind
Copy link
Contributor

The buffer part of this was landed recently in 6ad1f7b. @fansworld-claudio could you update this PR?

@claudiorodriguez
Copy link
Contributor Author

@silverwind That should do it, I think

@jasnell
Copy link
Member

jasnell commented Feb 7, 2016

LGTM

@silverwind
Copy link
Contributor

LGTM. @fansworld-claudio can you rebase once more?

@claudiorodriguez
Copy link
Contributor Author

@silverwind Done

Changes the parsing of parameter types in the doc html gen
Links to either MDN or nodejs docs depending on type
See nodejs#4350
Fixes several type references in the docs so that the
doc html gen tool that parses them can put the correct
links in.
Changes css styling for the generated type links.
silverwind pushed a commit that referenced this pull request Feb 9, 2016
Changes the parsing of parameter types in the doc html gen
Links to either MDN or nodejs docs depending on type
See #4350

PR-URL: #4741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
silverwind pushed a commit that referenced this pull request Feb 9, 2016
Fixes several type references in the docs so that the
doc html gen tool that parses them can put the correct
links in.

Changes css styling for the generated type links.

PR-URL: #4741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@silverwind
Copy link
Contributor

Thanks! Landed in e517efa and 47e926a.

@silverwind silverwind closed this Feb 9, 2016
@Trott
Copy link
Member

Trott commented Feb 9, 2016

This change does not lint cleanly with the current linting rules. I'll submit a PR to fix.

Trott added a commit to Trott/io.js that referenced this pull request Feb 9, 2016
@silverwind
Copy link
Contributor

Whoops, thanks for picking up the slack @Trott.

silverwind pushed a commit that referenced this pull request Feb 9, 2016
Refs: #4741 (comment)
PR-URL: #5161
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 10, 2016
Changes the parsing of parameter types in the doc html gen
Links to either MDN or nodejs docs depending on type
See #4350

PR-URL: #4741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 10, 2016
Fixes several type references in the docs so that the
doc html gen tool that parses them can put the correct
links in.

Changes css styling for the generated type links.

PR-URL: #4741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 10, 2016
Refs: #4741 (comment)
PR-URL: #5161
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@MylesBorins
Copy link
Contributor

This change is not landing nicely into LTS. We need to get a backlog of commits updated for this to work

MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
Changes the parsing of parameter types in the doc html gen
Links to either MDN or nodejs docs depending on type
See #4350

PR-URL: #4741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
Fixes several type references in the docs so that the
doc html gen tool that parses them can put the correct
links in.

Changes css styling for the generated type links.

PR-URL: #4741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
Refs: #4741 (comment)
PR-URL: #5161
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
Changes the parsing of parameter types in the doc html gen
Links to either MDN or nodejs docs depending on type
See #4350

PR-URL: #4741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
Fixes several type references in the docs so that the
doc html gen tool that parses them can put the correct
links in.

Changes css styling for the generated type links.

PR-URL: #4741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
Refs: #4741 (comment)
PR-URL: #5161
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Changes the parsing of parameter types in the doc html gen
Links to either MDN or nodejs docs depending on type
See #4350

PR-URL: #4741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Fixes several type references in the docs so that the
doc html gen tool that parses them can put the correct
links in.

Changes css styling for the generated type links.

PR-URL: #4741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Refs: #4741 (comment)
PR-URL: #5161
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants