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

http2: order declarations in core.js #21689

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 6, 2018

Not sure how this will be received because maybe there's an ordering in there already that I'm not detecting, but I find the lengthy declarations at the start of core.js kind of arbitrary and hard to follow. This brings some ordering to it, and hopefully makes it easier to follow or at least not any more difficult to follow.

Order declarations:

  • public modules in alphabetical order
  • internal modules in alphabetical order
  • process.binding() calls in alphabetical order
  • exports in alphabetical order
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Jul 6, 2018
@Trott
Copy link
Member Author

Trott commented Jul 6, 2018

Spurious lint error again. This is the second time this has happened to one of my pull requests. Something is strange with the auto-started lint jobs. /ping @maclover7

In the meantime, here's a full CI: https://ci.nodejs.org/job/node-test-pull-request/15751/

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

as long as all the tests pass seems good

@Trott
Copy link
Member Author

Trott commented Jul 9, 2018

Order declarations:

* public modules in alphabetical order
* internal modules in alphabetical order
* process.binding() calls in alphabetical order
* exports in alphabetical order
@Trott
Copy link
Member Author

Trott commented Jul 10, 2018

Had to resolve a conflict so here's CI again: https://ci.nodejs.org/job/node-test-pull-request/15785/

Trott added a commit to Trott/io.js that referenced this pull request Jul 10, 2018
Order declarations:

* public modules in alphabetical order
* internal modules in alphabetical order
* process.binding() calls in alphabetical order
* exports in alphabetical order

PR-URL: nodejs#21689
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jul 10, 2018

Landed in 11931ae

@Trott Trott closed this Jul 10, 2018
targos pushed a commit that referenced this pull request Jul 12, 2018
Order declarations:

* public modules in alphabetical order
* internal modules in alphabetical order
* process.binding() calls in alphabetical order
* exports in alphabetical order

PR-URL: #21689
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Jul 17, 2018
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
Order declarations:

* public modules in alphabetical order
* internal modules in alphabetical order
* process.binding() calls in alphabetical order
* exports in alphabetical order

PR-URL: nodejs#21689
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Sep 25, 2018
Order declarations:

* public modules in alphabetical order
* internal modules in alphabetical order
* process.binding() calls in alphabetical order
* exports in alphabetical order

PR-URL: nodejs#21689
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Order declarations:

* public modules in alphabetical order
* internal modules in alphabetical order
* process.binding() calls in alphabetical order
* exports in alphabetical order

PR-URL: nodejs#21689
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Order declarations:

* public modules in alphabetical order
* internal modules in alphabetical order
* process.binding() calls in alphabetical order
* exports in alphabetical order

Backport-PR-URL: #22850
PR-URL: #21689
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants