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.

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

lib: extract validateString validator #22101

Closed
wants to merge 1 commit into from
Closed

lib: extract validateString validator #22101

wants to merge 1 commit into from

Conversation

maclover7
Copy link
Contributor

Pulls out a common argument validator to internal/validators

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 3, 2018
ghost
ghost reviewed Aug 3, 2018
View reviewed changes
Copy link

@ghost ghost left a comment •
edited by ghost
Loading

Choose a reason for hiding this comment

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

What about trying to create another new function called "isEmptyString" to check whether your string value is empty:

function isEmptyString (s){
   return (typeof s === 'string' && s.trim() === '');
}

Most of your functions requires a param value that isn't null or empty.
Maybe when both of the things meet, we can continue going on.

@maclover7
Copy link
Contributor Author

Most of your functions requires a param value that isn't null or empty.

@Maledong Not sure if this is the case -- where do you see this check? I'd rather not add new validations in this PR, since that would be a semver-major change

@joyeecheung
Copy link
Member

joyeecheung commented Aug 3, 2018 •
edited
Loading

@Maledong Arguably s.trim() === '' is ERR_INVALID_ARG_VALUE and type of s !== 'string' is ERR_INVALID_ARG_TYPE so it may not make sense to mix the validations together. And yeah that'd be semver-major for a couple of APIs.

@joyeecheung
Copy link
Member

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

targos
targos approved these changes Aug 3, 2018
View reviewed changes
lib/net.js Outdated
if (typeof path !== 'string') {
throw new ERR_INVALID_ARG_TYPE('options.path', 'string', path);
}
validateString(path, 'path');
Copy link
Member

Choose a reason for hiding this comment

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

Name changed from "options.path" to "path". I'm on mobile so can't check which one is correct.

Copy link
Member

Choose a reason for hiding this comment

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

Technically both are valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think changing the name was a mistake, but going to put this back to options.path instead of path to make sure this is definitely not semver-major :)

@Trott
Copy link
Member

Trott commented Aug 4, 2018

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

(Didn't use Resume Build because windows-fanned failed and Resume Build does not cooperate with windows-fanned.)

@maclover7
Copy link
Contributor Author

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

@maclover7
Copy link
Contributor Author

updated @targos @lpinca PTAL

@lpinca
Copy link
Member

lpinca commented Aug 7, 2018

Still LGMT.

@maclover7
lib: extract validateString validator
fc8d944 
Pulls out a common argument validator to `internal/validators`
@maclover7
Copy link
Contributor Author

Landed in e570ae7, thank you for the reviews!

@maclover7 maclover7 closed this Aug 7, 2018
@maclover7 maclover7 deleted the jm-validatestring branch August 7, 2018 15:04
maclover7 added a commit that referenced this pull request Aug 7, 2018
@maclover7
lib: extract validateString validator
e570ae7 
Pulls out a common argument validator to `internal/validators`

PR-URL: #22101
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Aug 11, 2018
@maclover7 @targos
lib: extract validateString validator
193a048 
Pulls out a common argument validator to `internal/validators`

PR-URL: #22101
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@rvagg rvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
Closed
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ZYSzys ZYSzys mentioned this pull request Dec 6, 2018
Closed
2 tasks
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 9, 2018
@ZYSzys @Trott
module: use validateString in modules/cjs
4dc10ac 
PR-URL: nodejs#24863
Refs: nodejs#22101
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@ZYSzys ZYSzys mentioned this pull request Dec 11, 2018
Closed
2 tasks
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 14, 2018
@ZYSzys @Trott
lib: refactor argument validation using validateString
a35bd62 
PR-URL: nodejs#24960
Refs: nodejs#22101
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 17, 2018
@ZYSzys @BethGriggs
module: use validateString in modules/cjs
2a11e6a 
PR-URL: #24863
Refs: #22101
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
@ZYSzys @MylesBorins
lib: refactor argument validation using validateString
96bdd47 
PR-URL: #24960
Refs: #22101
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@ZYSzys @refack
module: use validateString in modules/cjs
70adfe1 
PR-URL: nodejs#24863
Refs: nodejs#22101
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@ZYSzys @refack
lib: refactor argument validation using validateString
6033f5f 
PR-URL: nodejs#24960
Refs: nodejs#22101
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
@ZYSzys @BethGriggs
module: use validateString in modules/cjs
229f901 
PR-URL: #24863
Refs: #22101
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 20, 2019
@ZYSzys @BethGriggs
module: use validateString in modules/cjs
153f501 
PR-URL: #24863
Refs: #22101
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@ZYSzys @rvagg
module: use validateString in modules/cjs
6cede0c 
PR-URL: #24863
Refs: #22101
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@maclover7 @nodejs-github-bot @joyeecheung @Trott @lpinca @apapirovski @jasnell @targos @cjihrig @trivikr