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: refactor to reuse validators #38608

Closed
wants to merge 2 commits into from

Conversation

pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented May 9, 2021

No description provided.

@github-actions github-actions bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 9, 2021
lib/_tls_wrap.js Outdated Show resolved Hide resolved
lib/internal/perf/timerify.js Show resolved Hide resolved
lib/internal/process/per_thread.js Outdated Show resolved Hide resolved
lib/util.js Show resolved Hide resolved
lib/vm.js Outdated Show resolved Hide resolved
lib/wasi.js Outdated Show resolved Hide resolved
lib/zlib.js Outdated
@@ -71,6 +71,7 @@ const {
const { owner_symbol } = require('internal/async_hooks').symbols;
const {
validateFunction,
validateNumber
Copy link
Member

Choose a reason for hiding this comment

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

nit: trailing comma

Copy link
Contributor Author

@pd4d10 pd4d10 May 9, 2021

Choose a reason for hiding this comment

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

OK. But the lint seems to pass. Shall we add this to the lint rule?

Copy link
Member

Choose a reason for hiding this comment

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

OK. But the lint seems passed. Shall we add this to the lint rule?

It would be nice if the lint rule could be added

@RaisinTen
Copy link
Contributor

Why is this happening? 👀

make[2]: Leaving directory '/home/runner/work/node/node/benchmark/napi/type-tag-check/build'
npm ERR! normalizeEncoding is not a function
make[1]: *** [Makefile:689: tools/doc/node_modules] Error 1

lib/events.js Outdated Show resolved Hide resolved
lib/_tls_wrap.js Outdated Show resolved Hide resolved
lib/_http_agent.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@pd4d10
Copy link
Contributor Author

pd4d10 commented May 16, 2021

It seems that we should leave lib/util.js as is since lib/internal/validators.js depends on it, which is the cause of test failure.

Updated

@aduh95
Copy link
Contributor

aduh95 commented May 16, 2021

It seems that we should leave lib/util.js as is since lib/internal/validators.js depends on it, which is the cause of test failure.

Most likely the error comes from lib/internal/util.js, not lib/utils.js: adding the require call there is creating a circular dependency hence the not defined error.

@pd4d10
Copy link
Contributor Author

pd4d10 commented May 16, 2021

It seems that we should leave lib/util.js as is since lib/internal/validators.js depends on it, which is the cause of test failure.

Most likely the error comes from lib/internal/util.js, not lib/utils.js: adding the require call there is creating a circular dependency hence the not defined error.

Yeah, you are right. I misread the file name.

Updated

@aduh95
Copy link
Contributor

aduh95 commented May 16, 2021

FYI force pushing makes the review process a bit harder, if you could prefer fixup commits instead that'd be nice. FYI you can update your local branch using git pull [email protected]:pd4d10/node.git patch-lib-validator.
Could you please reaply 1648190, it seems it was lost during the force-push.

Co-authored-by: Antoine du Hamel <[email protected]>
@pd4d10
Copy link
Contributor Author

pd4d10 commented May 16, 2021

FYI force pushing makes the review process a bit harder, if you could prefer fixup commits instead that'd be nice. FYI you can update your local branch using git pull [email protected]:pd4d10/node.git patch-lib-validator.
Could you please reaply 1648190, it seems it was lost during the force-push.

OK, get it. Updated

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 16, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 16, 2021

pd4d10 added a commit to pd4d10/node that referenced this pull request May 18, 2021
The advantage of doing this is that `eslint --fix` can automatically add
trailing commas, which avoids wasting time on manual formatting

Refs: nodejs#38701
Refs: nodejs#38608 (review)
jasnell pushed a commit that referenced this pull request May 19, 2021
PR-URL: #38608
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented May 19, 2021

Landed in 5d7b6c2

@jasnell jasnell closed this May 19, 2021
@pd4d10 pd4d10 deleted the patch-lib-validator branch May 19, 2021 16:26
danielleadams pushed a commit that referenced this pull request May 31, 2021
PR-URL: #38608
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request May 31, 2021
@richardlau
Copy link
Member

This appears to depend on #37045 so adding a matching backport label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants