Skip to content

doc: add added: information for globals - #8901

Closed
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:add/added-globals
Closed

doc: add added: information for globals#8901
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:add/added-globals

Conversation

@lpinca

@lpincalpinca commented Oct 3, 2016

Copy link
Copy Markdown
Member
Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Ref: #6578

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. globals labels Oct 3, 2016
addaleax
addaleax approved these changes Oct 3, 2016
View reviewed changes

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

silverwind
silverwind suggested changes Oct 5, 2016
View reviewed changes

@silverwindsilverwind left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Care to leave out the whitespace changes? IIRC, some people prefer the double space style to start sentences. It's a stylistic issue that should probably be discussed beforehand and then done once everywhere.

Comment threaddoc/api/globals.md Outdated

@silverwindsilverwindOct 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe we should lint for these, if it's possible.

@lpincalpincaOct 5, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No strong opinion on this so yeah I can revert this change but when rendered it uses a single space anyway so I don't see the point of using 2+.

@lpincalpincaOct 5, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done.

@silverwind

silverwind commented Oct 5, 2016

Copy link
Copy Markdown
Contributor

Should we document the deprecated GLOBAL and root?

@Fishrock123

Fishrock123 commented Oct 5, 2016

Copy link
Copy Markdown
Contributor

@silverwind I'd say probably not... what point is there now?

@silverwind

silverwind commented Oct 5, 2016

Copy link
Copy Markdown
Contributor

Just for historical purpose I guess. They were deprecated in 6.0.0 (#1838), by the way.

@lpinca

lpinca commented Oct 6, 2016

Copy link
Copy Markdown
MemberAuthor

@silverwind this means re-adding GLOBAL and root in the doc as they are no longer there. I don't know if it makes sense.

@addaleax

addaleax commented Oct 6, 2016

Copy link
Copy Markdown
Member

Yeah, let’s not add GLOBAL and root to the doc, it’s good that they are undocumented when they are deprecated anyway.

@silverwind

silverwind commented Oct 6, 2016

Copy link
Copy Markdown
Contributor

Okay, lets leave them out. LGTM

jasnell
jasnell approved these changes Oct 6, 2016
View reviewed changes

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@addaleaxaddaleax self-assigned this Oct 6, 2016
@addaleax

addaleax commented Oct 6, 2016

Copy link
Copy Markdown
Member

I’ll start landing this:

  • Approvals (LGTM): 3
  • No objections
  • The PR has been open for the minimum time of 48 or 72 hours
  • All of the requested changes have been made
  • CI is unnecessary

@addaleax

addaleax commented Oct 6, 2016

Copy link
Copy Markdown
Member

Landed in bd0bedb

@addaleaxaddaleax closed this Oct 6, 2016
addaleax pushed a commit that referenced this pull request Oct 6, 2016
@lpinca@addaleax
doc: add added: information for globals
bd0bedb
Ref: #6578
PR-URL: #8901
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax

addaleax commented Oct 6, 2016

Copy link
Copy Markdown
Member

Heads up, this doesn’t land cleanly on v4.x-staging, so a backport would be necessary for that.

@lpinca
lpinca deleted the add/added-globals branch October 7, 2016 05:40
lpinca added a commit to lpinca/node that referenced this pull request Oct 7, 2016
@lpinca
doc: add added: information for globals
c24e44c
Ref: nodejs#6578
PR-URL: nodejs#8901
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@lpincalpinca mentioned this pull request Oct 7, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Oct 7, 2016
@lpincaMyles Borins
doc: add added: information for globals
ab08cc2
Ref: #6578
Ref: #8964
PR-URL: #8901
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Oct 10, 2016
@lpinca@jasnell
doc: add added: information for globals
8ec9582
Ref: #6578
PR-URL: #8901
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
@lpincaMyles Borins
doc: add added: information for globals
4da9ddd
Ref: #6578
Ref: #8964
PR-URL: #8901
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@lpinca@Fishrock123
doc: add added: information for globals
0bdc363
Ref: #6578
PR-URL: #8901
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
@lpinca@rvagg
doc: add added: information for globals
6fb2ef4
Ref: #6578
Ref: #8964
PR-URL: #8901
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
@lpinca@MylesBorins
doc: add added: information for globals
da3b938
Ref: #6578
Ref: #8964
PR-URL: #8901
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorinsMylesBorins mentioned this pull request Oct 26, 2016
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
+1 more reviewer
@silverwindsilverwindsilverwind approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@addaleaxaddaleax

Labels

docIssues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lpinca@silverwind@Fishrock123@addaleax@jasnell@nodejs-github-bot