lib: refactor validators by BridgeAR · Pull Request #26809 · nodejs/node (original) (raw)

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.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits9 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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

Choose a reason for hiding this comment

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

LGTM

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

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.