Skip to content

lib: refactor to reuse validators#38608

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.

Sign up for GitHub

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

Jump to bottom
Closed
pd4d10 wants to merge 2 commits into from
Closed

lib: refactor to reuse validators #38608

pd4d10 wants to merge 2 commits into from

Conversation

@pd4d10
Copy link
Contributor

@pd4d10pd4d10 commented May 9, 2021

No description provided.

@github-actionsgithub-actionsbot 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
@pd4d10pd4d10 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

@LxxyxLxxyxMay 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
ContributorAuthor

@pd4d10pd4d10May 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

@LxxyxLxxyxMay 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 functionmake[1]: *** [Makefile:689: tools/doc/node_modules] Error 1

@pd4d10pd4d10force-pushed the patch-lib-validator branch from f55af36 to 92726a9CompareMay 16, 2021 11:23
@pd4d10pd4d10 requested a review from LxxyxMay 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/

@pd4d10pd4d10force-pushed the patch-lib-validator branch from 8659f35 to fce76feCompareMay 16, 2021 15:36
@pd4d10
Copy link
ContributorAuthor

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.

@pd4d10pd4d10force-pushed the patch-lib-validator branch from 1648190 to 48d020cCompareMay 16, 2021 15:49
@pd4d10
Copy link
ContributorAuthor

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.

@pd4d10@aduh95
Apply suggestions from code review
9b29dcc
Co-authored-by: Antoine du Hamel <[email protected]>
@pd4d10
Copy link
ContributorAuthor

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

nodejs-github-bot commented May 16, 2021

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

@aduh95aduh95 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)
@pd4d10pd4d10 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 <[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

@jasnelljasnell closed this May 19, 2021
@pd4d10pd4d10 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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: James M Snell <[email protected]>
@danielleadamsdanielleadams 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@TrottTrottTrott approved these changes

@LxxyxLxxyxLxxyx approved these changes

@aduh95aduh95aduh95 approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs 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