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: update and modernize examples in api docs #4282

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 14, 2015

  • Use single quotes consistently
  • Modernize examples to use template strings and arrow funcs
  • Fix typos
  • Fix a few line wrap issues

/cc @nodejs/http

@jasnell jasnell added http Issues or PRs related to the http subsystem. doc Issues and PRs related to the documentations. labels Dec 14, 2015
@@ -103,7 +103,7 @@ of these values set to their respective defaults.
To configure any of them, you must create your own [`http.Agent`][] object.

```javascript
var http = require('http');
const http = require('http');
Copy link
Contributor

Choose a reason for hiding this comment

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

should we do a pass at replacing all instances of require in docs with const?

Copy link
Member Author

Choose a reason for hiding this comment

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

Incrementally, perhaps. I don't think it's critical enough to do all at once

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. my only thought on this is that it might be weird to have things in the docs inconsistent, but it makes sense to avoid massive churn

Copy link
Contributor

Choose a reason for hiding this comment

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

It might actually be better to have huge docs only churn in a single commit, instead of 32 commits. People are a lot less likely to bisect and blame the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking it over, I agree. It's not a huge chunk of work. Already have it
mostly done. Will update this PR with the other updates.
On Dec 14, 2015 4:51 PM, "Colin Ihrig" [email protected] wrote:

In doc/api/http.markdown
#4282 (comment):

@@ -103,7 +103,7 @@ of these values set to their respective defaults.
To configure any of them, you must create your own [http.Agent][] object.

-var http = require('http');
+const http = require('http');

It might actually be better to have huge docs only churn in a single
commit, instead of 32 commits #3662.
People are a lot less likely to bisect and blame the docs.


Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/4282/files#r47583137.

@MylesBorins
Copy link
Contributor

LGTM. Small comment regarding using const in docs above, but that is not relevant to this landing

@mscdex
Copy link
Contributor

mscdex commented Dec 14, 2015

Argh, the ES6 inline functions continue to creep in!

@jasnell
Copy link
Member Author

jasnell commented Dec 15, 2015

lol ... they're not the prettiest thing in the world but it's the direction things are heading

@@ -64,10 +64,10 @@ a `'close'` event or a special `'agentRemove'` event. This means that if
you intend to keep one HTTP request open for a long time and don't
want it to stay in the pool you can do something along the lines of:

http.get(options, function(res) {
http.get(options, (res)=> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have any linting rules for arrow functions yet, but from a quick code search, it looks like the convention has been to put spaces on each side of the arrow. So, in this case (res) => {.

Copy link
Member

Choose a reason for hiding this comment

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

agreed, I'm only seeing (args) => { in use in the wild at the moment with the extra space (not that I'm looking very widely)

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case it's probably fine to omit the parens, as well — http.get(options, res => {

Copy link
Member

Choose a reason for hiding this comment

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

my vote would be to never omit parens to be explicit what's going on, I personally find the paren-free version little too terse

Copy link
Contributor

Choose a reason for hiding this comment

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

It's definitely a brand new 🚲🏡; I don't feel strongly enough either way to propose setting a hard-and-fast rule for it. In my own projects, I lean towards minimizing characters in code examples to give other info more room, but two characters is not a huge amount of savings so I'm comfortable leaving this as a personal preference of the doc author for the time being.

@jasnell
Copy link
Member Author

jasnell commented Dec 15, 2015

@thealphanerd @cjihrig ... ok, just pushed a big update. Fixed up all of the doc examples at once.

@jasnell jasnell changed the title doc: update examples in http.markdown doc: update and modernize examples in api docs Dec 15, 2015
@jasnell
Copy link
Member Author

jasnell commented Dec 15, 2015

@nodejs/documentation

@chrisdickinson
Copy link
Contributor

Are you comfortable asserting that the code examples still work as authored? If so, this LGTM pending arrow function style nits!

@jasnell
Copy link
Member Author

jasnell commented Dec 15, 2015

I'm going to be going through and testing the various cases. Gimme a day to
get through them. Pretty certain everything is good tho
On Dec 14, 2015 9:44 PM, "Chris Dickinson" [email protected] wrote:

Are you comfortable asserting that the code examples still work as
authored? If so, this LGTM pending arrow function style nits!


Reply to this email directly or view it on GitHub
#4282 (comment).

@chrisdickinson
Copy link
Contributor

@jasnell Awesome, thanks — great work!

@Fishrock123
Copy link
Contributor

While we're at it, is there a good reason to keep semicolons in these?

@cjihrig
Copy link
Contributor

cjihrig commented Dec 15, 2015

LGTM

@@ -135,4 +135,4 @@ Custom error validation:
[`assert.throws()`]: #assert_assert_throws_block_error_message
[`Error`]: errors.html#errors_class_error
[`RegExp`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions
[`TypeError`]: errors.html#errors_class_typeerror
Copy link
Contributor

Choose a reason for hiding this comment

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

what change happened here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a clue. Likely a whitespace change. Will fix.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 15, 2015

New changes look good. Left one comment directly on the commit instead of the PR.

@jasnell
Copy link
Member Author

jasnell commented Dec 16, 2015

const can be used without 'use strict', it's let that can't be used without it.

bash-3.2$ cat ~/tmp/test.js
const m = 1;
let n = 1;
bash-3.2$ node ~/tmp.test
module.js:339
    throw err;
    ^

Error: Cannot find module '/Users/james/tmp.test'
    at Function.Module._resolveFilename (module.js:337:15)
    at Function.Module._load (module.js:287:25)
    at Function.Module.runMain (module.js:467:10)
    at startup (node.js:134:18)
    at node.js:961:3
bash-3.2$ 

@Trott
Copy link
Member

Trott commented Dec 16, 2015

Uh, yeah, never mind my previous comments about strict mode...

@jasnell
Copy link
Member Author

jasnell commented Dec 16, 2015

Pushed update to pull the "fix" for #4296 back out. That change isn't necessary for master or v5

@jasnell
Copy link
Member Author

jasnell commented Dec 16, 2015

@cjihrig ... updated the 0022 to 0o022 per your suggestion

@jasnell
Copy link
Member Author

jasnell commented Dec 16, 2015

I marked this for LTS watch but it likely won't land cleanly. If/when it lands, I'll put together a modified version for v4.x-staging

@jasnell
Copy link
Member Author

jasnell commented Dec 16, 2015

I'd like to get this landed soon but would appreciate a bit more eyes on it. /cc @nodejs/documentation

@jasnell
Copy link
Member Author

jasnell commented Dec 16, 2015

Rebased and updated...

* Use single quotes consistently
* Modernize examples to use template strings and arrow funcs
* Fix a few typos
Additional example edits for consistency
@jasnell
Copy link
Member Author

jasnell commented Dec 17, 2015

Getting this landed momentarily

jasnell added a commit that referenced this pull request Dec 17, 2015
* Use single quotes consistently
* Modernize examples to use template strings and arrow funcs
* Fix a few typos
* Example edits for consistency

PR-URL: #4282
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Dec 17, 2015

Landed in 9b21119

@jasnell jasnell closed this Dec 17, 2015
@jasnell jasnell mentioned this pull request Dec 17, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Dec 22, 2015
* Use single quotes consistently
* Modernize examples to use template strings and arrow funcs
* Fix a few typos
* Example edits for consistency

PR-URL: nodejs#4282
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell added a commit that referenced this pull request Dec 30, 2015
* Use single quotes consistently
* Modernize examples to use template strings and arrow funcs
* Fix a few typos
* Example edits for consistency

PR-URL: #4282
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
* Use single quotes consistently
* Modernize examples to use template strings and arrow funcs
* Fix a few typos
* Example edits for consistency

PR-URL: nodejs#4282
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
* Use single quotes consistently
* Modernize examples to use template strings and arrow funcs
* Fix a few typos
* Example edits for consistency

PR-URL: #4282
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
* Use single quotes consistently
* Modernize examples to use template strings and arrow funcs
* Fix a few typos
* Example edits for consistency

PR-URL: nodejs#4282
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Colin Ihrig <[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. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants