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

feat(helpers): allow empty string in fake #1679

Merged
merged 6 commits into from
Dec 25, 2022
Merged

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Dec 23, 2022

Fixes #1457

Allows faker.helpers.fake('') => ''

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug m: helpers Something is referring to the helpers module labels Dec 23, 2022
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Dec 23, 2022
@ST-DDT ST-DDT requested review from a team December 23, 2022 12:03
@ST-DDT ST-DDT self-assigned this Dec 23, 2022
@codecov
Copy link

codecov bot commented Dec 23, 2022

Codecov Report

Merging #1679 (8b4d962) into next (aa77888) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1679      +/-   ##
==========================================
- Coverage   99.64%   99.64%   -0.01%     
==========================================
  Files        2240     2240              
  Lines      240106   240098       -8     
  Branches     1072     1068       -4     
==========================================
- Hits       239247   239239       -8     
  Misses        838      838              
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/helpers/index.ts 99.14% <100.00%> (-0.01%) ⬇️

import-brain
import-brain previously approved these changes Dec 23, 2022
test/helpers.spec.ts Outdated Show resolved Hide resolved
Shinigami92
Shinigami92 previously approved these changes Dec 23, 2022
Shinigami92
Shinigami92 previously approved these changes Dec 23, 2022
import-brain
import-brain previously approved these changes Dec 23, 2022
@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Dec 23, 2022

Is this a breaking change? I (as a user) could have the following case:

function foo(unvalidatedInput: string) {
  try {
    return fake(unvalidatedInput);
  } catch {
    // I use the fact, that faker.helpers.fake() can't handle empty strings
    return myDefaultValue;
  }
}

Just want to get the labels correct and PR title.

@ST-DDT
Copy link
Member Author

ST-DDT commented Dec 24, 2022

If you are relying on the fact that the method throws on empty strings, then yes it is a breaking change.

But:

  • We didn't even have a test for that before feat(helpers): fake from array #1453 (merged today)
  • We never specified that we would throw in case of an empty string
  • We explicitly told the user not to use empty strings

aka it was explicitly undefined behavior before

I'm fine with marking this as breaking or not.

Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

We explicitly told the user not to use empty strings

That's a good point. Then it's not breaking IMO.

@ST-DDT ST-DDT enabled auto-merge (squash) December 25, 2022 14:25
@ST-DDT ST-DDT merged commit f0d2ffb into next Dec 25, 2022
@Shinigami92 Shinigami92 deleted the feat/fake/empty-string branch December 25, 2022 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow passing empty string to fake()
4 participants