| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
@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 |
Sorry, something went wrong.
|
It would be nice to get some reviews here. This is a code cleanup that I wanted to do after working on #26738. |
Sorry, something went wrong.
|
Rebased due to conflicts. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
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.
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.
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.
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.
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.
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.
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.
This renames the parameters for clarity and removes the check for undefined encoding. That will always default to `utf8`.
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.
|
Rebased due to conflicts. |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
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>
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>
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>
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>
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>
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>
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>
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>
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>
|
This depends on a semver-major PR and needs to be manually backported to v11.x-staging |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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