Skip to content

lib: refactor to reuse validators#38608

Closed
pd4d10 wants to merge 2 commits intonodejs:masterfrom
pd4d10:patch-lib-validator
Closed

lib: refactor to reuse validators#38608
pd4d10 wants to merge 2 commits intonodejs:masterfrom
pd4d10:patch-lib-validator

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
@pd4d10 pd4d10 mentioned this pull request May 9, 2021
Open
Lxxyx
Lxxyx reviewed May 9, 2021
View reviewed changes
lib/zlib.js Outdated
const { owner_symbol } = require('internal/async_hooks').symbols;
const {
validateFunction,
validateNumber
Copy link
Member

@Lxxyx Lxxyx May 9, 2021

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 •
edited
Loading

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

@Lxxyx Lxxyx 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 passed. Shall we add this to the lint rule?

It would be nice if the lint rule could be added

@RaisinTen
Copy link
Member

RaisinTen commented May 9, 2021

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

@pd4d10 pd4d10 force-pushed the patch-lib-validator branch from f55af36 to 92726a9 Compare May 16, 2021 11:23
@pd4d10 pd4d10 requested a review from Lxxyx May 16, 2021 11:24
aduh95
aduh95 reviewed May 16, 2021
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 16, 2021

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

@pd4d10 pd4d10 force-pushed the patch-lib-validator branch from 8659f35 to fce76fe Compare May 16, 2021 15:36
@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 pd4d10 force-pushed the patch-lib-validator branch from 1648190 to 48d020c Compare May 16, 2021 15:49
@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 git@github.com:pd4d10/node.git patch-lib-validator.
Could you please reaply 1648190, it seems it was lost during the force-push.

@pd4d10 @aduh95
Apply suggestions from code review
9b29dcc 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@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 git@github.com: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

nodejs-github-bot commented May 16, 2021

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

@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 •
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/38140/ 💚

pd4d10 added a commit to pd4d10/node that referenced this pull request May 18, 2021
@pd4d10
tools: update ESLint config comma-dangle to always-multiline
794fae8 
The advantage of doing this is that `eslint --fix` can automatically add
trailing commas, which avoids wasting time on manual formatting

Refs: https://github.com/nodejs/node/discussions/38701
Refs: nodejs#38608 (review)
@pd4d10 pd4d10 mentioned this pull request May 18, 2021
Closed
jasnell pushed a commit that referenced this pull request May 19, 2021
@pd4d10 @jasnell
lib: refactor to reuse validators
5d7b6c2 
PR-URL: #38608
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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
@pd4d10 @danielleadams
lib: refactor to reuse validators
2268d1c 
PR-URL: #38608
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request May 31, 2021
Merged
@richardlau
Copy link
Member

richardlau commented Jul 16, 2021

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

Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@Lxxyx Lxxyx Lxxyx approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@pd4d10 @RaisinTen @nodejs-github-bot @aduh95 @jasnell @richardlau @Trott @Lxxyx