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

lib: minor cleanup #25149

Closed
wants to merge 4 commits into from

Conversation

@BridgeAR
Copy link
Member

commented Dec 20, 2018

Just two minor things I stumbled upon while looking at other things.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
lib/util.js Show resolved Hide resolved
@lpinca
lpinca approved these changes Dec 20, 2018

@BridgeAR BridgeAR force-pushed the BridgeAR:remove-obsolete-call branch from 2c91690 to 3ceea0a Dec 23, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Dec 23, 2018

PTAL

I moved the types part into the bootstrapping part. That seemed like the most natural place for it.

CI https://ci.nodejs.org/job/node-test-pull-request/19761/
CI https://ci.nodejs.org/job/node-test-pull-request/19762/

BridgeAR added 2 commits Dec 20, 2018
bootstrap: make sure all type checks exist early
Combine all type checks on the internal types binding during
bootstrapping. This makes sure all of those checks exist early on
and not only after loading the util module.

@BridgeAR BridgeAR force-pushed the BridgeAR:remove-obsolete-call branch from 3ceea0a to 8c06fd3 Dec 23, 2018

@Trott

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

CI failures are sufficiently extensive that I'm going to remove the author ready label. Feel free to re-add it as soon as appropriate. Not objecting. Just keeping it out of my search results until it's actually ready. :-D

@Trott Trott removed the author ready label Dec 23, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Dec 23, 2018

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved
fixup: refactor internal types
Now all type checks should be required loading 'internal/util/types'.
@Trott

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Dec 27, 2018

@lpinca @addaleax @devsnek PTAL (I changed the code significantly enough that confirming the LG would be good).

@lpinca

This comment has been minimized.

Copy link
Member

commented Dec 27, 2018

Still LGTM.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 27, 2018
lib: expose all type checks from the internal types module
Combine all type checks on the internal types module and do not use
the types binding anywhere else anymore. This makes sure all of those
checks exist when required.

PR-URL: nodejs#25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 27, 2018
console: use spread notation instead of Object.assign
PR-URL: nodejs#25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Dec 27, 2018

Landed in 5ac30c9 and 4ea2c1c

@BridgeAR BridgeAR closed this Dec 27, 2018

targos added a commit that referenced this pull request Jan 1, 2019
console: use spread notation instead of Object.assign
PR-URL: #25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@addaleax

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

The lib: commit does not apply cleanly, so:

Should this be backported to v11.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 9, 2019
lib: expose all type checks from the internal types module
Combine all type checks on the internal types module and do not use
the types binding anywhere else anymore. This makes sure all of those
checks exist when required.

PR-URL: nodejs#25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 9, 2019
lib: expose all type checks from the internal types module
Combine all type checks on the internal types module and do not use
the types binding anywhere else anymore. This makes sure all of those
checks exist when required.

PR-URL: nodejs#25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@BridgeAR BridgeAR referenced this pull request Jan 9, 2019
4 of 4 tasks complete
BridgeAR added a commit that referenced this pull request Jan 10, 2019
lib: expose all type checks from the internal types module
Combine all type checks on the internal types module and do not use
the types binding anywhere else anymore. This makes sure all of those
checks exist when required.

PR-URL: #25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 11, 2019
lib: expose all type checks from the internal types module
Combine all type checks on the internal types module and do not use
the types binding anywhere else anymore. This makes sure all of those
checks exist when required.

PR-URL: nodejs#25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
addaleax added a commit that referenced this pull request Jan 14, 2019
lib: expose all type checks from the internal types module
Combine all type checks on the internal types module and do not use
the types binding anywhere else anymore. This makes sure all of those
checks exist when required.

PR-URL: #25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>

Backport-PR-URL: #25446
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
lib: expose all type checks from the internal types module
Combine all type checks on the internal types module and do not use
the types binding anywhere else anymore. This makes sure all of those
checks exist when required.

PR-URL: nodejs#25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
console: use spread notation instead of Object.assign
PR-URL: nodejs#25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
lib: expose all type checks from the internal types module
Combine all type checks on the internal types module and do not use
the types binding anywhere else anymore. This makes sure all of those
checks exist when required.

PR-URL: nodejs#25149
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>

Backport-PR-URL: nodejs#25446
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

@targos targos added this to Backported in v11.x Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
7 participants
You can’t perform that action at this time.