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.

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

Missing values should default to empty strings for errorOnRegex #45

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Missing values should default to empty strings for errorOnRegex #45

merged 1 commit into from
Sep 30, 2020

Conversation

FokkeZB
Copy link
Contributor

@FokkeZB FokkeZB commented Sep 25, 2020

IMO the RegExp().test() for errorOnRegex should default missing - or actually any non-string - values to "".

Currently, ^(true|undefined)$ is required to require a variable to either be "true" or not set. This feels hacky as it relies on RegExp().test() casting non-string values to a string, in which case undefined becomes "undefined".

As environment variables should always be strings, I think it's better to default non-strings to "" so that you can use ^(true)?$ to require a value to either be "true" or not set.

Without the change in src/index.js the newly added test fails with:

      AssertionError: expected [Function: runTest] to throw error including 'REGEX MISMATCH: TEST_MISSING_REQUIRED' but got 'REGEX MISMATCH: TEST_MISSING_OPTIONAL, TEST_MISSING_REQUIRED'
      + expected - actual

      -REGEX MISMATCH: TEST_MISSING_OPTIONAL, TEST_MISSING_REQUIRED
      +REGEX MISMATCH: TEST_MISSING_REQUIRED

Also see #41

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 61b99ed on FokkeZB:RegExp-String into 43e550d on keithmorris:develop.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 61b99ed on FokkeZB:RegExp-String into 43e550d on keithmorris:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 61b99ed on FokkeZB:RegExp-String into 43e550d on keithmorris:develop.

@keithmorris
Copy link
Owner

@FokkeZB This makes complete sense. Thank you for the PR. I will merge and release shortly.

@keithmorris keithmorris merged commit 084f213 into keithmorris:develop Sep 30, 2020
@FokkeZB FokkeZB deleted the RegExp-String branch October 1, 2020 07:56
@FokkeZB FokkeZB mentioned this pull request Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants