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: refactor validators #26809

Closed
wants to merge 9 commits into from
Closed

lib: refactor validators #26809

wants to merge 9 commits into from

Conversation

BridgeAR
Copy link
Member

This makes sure our validator functions actually only validate and have no side effects. Please have a look at the individual commit messages for further details.

It is just a code cleanup and no behavior change.

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

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2987/pipeline

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 20, 2019
@BridgeAR BridgeAR mentioned this pull request Mar 20, 2019
Closed
4 tasks
@BridgeAR
Copy link
Member Author

It would be nice to get some reviews here. This is a code cleanup that I wanted to do after working on #26738.

@BridgeAR
Copy link
Member Author

Rebased due to conflicts.

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

mcollina
mcollina approved these changes Mar 24, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 24, 2019
@BridgeAR
Copy link
Member Author

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 25, 2019 •
edited by BridgeAR
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/21883/ ✔️

@BridgeAR
lib: rename validateMode to parseMode
aea5647 
The function did not only validate the mode but it returns a new
value depending on the input. Thus `validate` did not seem to be an
appropriate name.
@BridgeAR
lib: remove return values from validation functions
c1d5d17 
This makes sure the validation functions do not cause any side
effects. Validation functions should ideally only validate the input
without any other effect. Since the input value must be known from
the callee, there is no reason to return the input value.
@BridgeAR
timers: rename validateTimerDuration to getTimerDuration
5f76b83 
The function did not only validate the timer but it caused side
effects like a warning and potentially returned a different value
than the input value. Thus the name `validate` did not seem to be
appropriate.
@BridgeAR
child_process: rename _validateStdtio to getValidStdio
0ec9c67 
The name indicated only validation while it did much more and it
returned a different value to the callee function. The underscore
was also not necessary as the function is internal one way or the
other.
@BridgeAR
http2: remove side effects from validateSettings
f1a2bd5 
The function did not only validate the input so far but it also made
a copy of the input object and returned that copy to the callee
function. That copy was not necessary for all call sites and it was
not obvious that the function did not only validate the input but
that it also returned a copy of it. This makes sure the function does
nothing more than validation and copying is happening in the callee
function when required.
@BridgeAR
url: refactor validateHostname
641e65d 
This function did not only validate the input but it returned a new
value in case the hostname was valid. This simplifies the function
by always returning the required value, no matter if it is valid or
invalid, so the callee site does not have to check that anymore. On
top the function is renamed to `getHostname` to make clear that the
function does not only validate the input but it also returns a new
value.
@BridgeAR
http2: rename function for clarity
373c45b 
The function does not only validate the input but it causes side
effects by adding default options to the input object in case the
option is not set.
@BridgeAR
crypto: remove obsolete encoding check
047c840 
This renames the parameters for clarity and removes the check for
undefined encoding. That will always default to `utf8`.
@BridgeAR
lib: consolidate arrayBufferView validation
595fd96 
There are lots of places that validate for arrayBufferView and we
have multiple functions that do the same thing. Instead, move the
validation into `internal/validators` so all files can use that
instead.

There are more functions throughout the code that do the same but
it takes some more work to fully consolidate all of those.
@BridgeAR
Copy link
Member Author

Rebased due to conflicts.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 27, 2019
@BridgeAR
lib: rename validateMode to parseMode
50a3fe2 
The function did not only validate the mode but it returns a new
value depending on the input. Thus `validate` did not seem to be an
appropriate name.

PR-URL: nodejs#26809
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 27, 2019
@BridgeAR
lib: remove return values from validation functions
6c913fb 
This makes sure the validation functions do not cause any side
effects. Validation functions should ideally only validate the input
without any other effect. Since the input value must be known from
the callee, there is no reason to return the input value.

PR-URL: nodejs#26809
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 27, 2019
@BridgeAR
timers: rename validateTimerDuration to getTimerDuration
9e8c9be 
The function did not only validate the timer but it caused side
effects like a warning and potentially returned a different value
than the input value. Thus the name `validate` did not seem to be
appropriate.

PR-URL: nodejs#26809
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 27, 2019
@BridgeAR
child_process: rename _validateStdtio to getValidStdio
28e2c37 
The name indicated only validation while it did much more and it
returned a different value to the callee function. The underscore
was also not necessary as the function is internal one way or the
other.

PR-URL: nodejs#26809
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 27, 2019
@BridgeAR
http2: remove side effects from validateSettings
ce26590 
The function did not only validate the input so far but it also made
a copy of the input object and returned that copy to the callee
function. That copy was not necessary for all call sites and it was
not obvious that the function did not only validate the input but
that it also returned a copy of it. This makes sure the function does
nothing more than validation and copying is happening in the callee
function when required.

PR-URL: nodejs#26809
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 27, 2019
@BridgeAR
url: refactor validateHostname
bbfa93a 
This function did not only validate the input but it returned a new
value in case the hostname was valid. This simplifies the function
by always returning the required value, no matter if it is valid or
invalid, so the callee site does not have to check that anymore. On
top the function is renamed to `getHostname` to make clear that the
function does not only validate the input but it also returns a new
value.

PR-URL: nodejs#26809
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 27, 2019
@BridgeAR
http2: rename function for clarity
92db780 
The function does not only validate the input but it causes side
effects by adding default options to the input object in case the
option is not set.

PR-URL: nodejs#26809
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 27, 2019
@BridgeAR
crypto: remove obsolete encoding check
751c92d 
This renames the parameters for clarity and removes the check for
undefined encoding. That will always default to `utf8`.

PR-URL: nodejs#26809
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 27, 2019
@BridgeAR
lib: consolidate arrayBufferView validation
7bddfcc 
There are lots of places that validate for arrayBufferView and we
have multiple functions that do the same thing. Instead, move the
validation into `internal/validators` so all files can use that
instead.

There are more functions throughout the code that do the same but
it takes some more work to fully consolidate all of those.

PR-URL: nodejs#26809
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in 9d854fb...7bddfcc 🎉

@BridgeAR BridgeAR closed this Mar 27, 2019
@targos
Copy link
Member

targos commented Mar 28, 2019

This depends on a semver-major PR and needs to be manually backported to v11.x-staging

This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@BridgeAR BridgeAR deleted the refactor-validators branch January 20, 2020 11:57
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

@mcollina mcollina mcollina 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BridgeAR @nodejs-github-bot @targos @mcollina @jasnell