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

The faker.phone.number() is returned differently than in the documentation. #3283

Closed
10 tasks done
sounmind opened this issue Nov 25, 2024 · 5 comments
Closed
10 tasks done
Labels
c: bug Something isn't working c: docs Improvements or additions to documentation has workaround Workaround provided or linked m: phone Something is referring to the phone module p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision

Comments

@sounmind
Copy link

sounmind commented Nov 25, 2024

Pre-Checks

Describe the bug

I read the documentation and thought that faker.phone.number() would return a string of the form xxx-xxx-xxxx, but it doesn't.

Need to supplement the documentation so that readers can fully expect the value returned by faker.phone.number().

Minimal reproduction code

image

Additional Context

No response

Environment Info

System:
  OS: macOS 14.1.1
  CPU: (8) arm64 Apple M2
  Memory: 111.69 MB / 16.00 GB
  Shell: 5.9 - /bin/zsh
Binaries:
  Node: 22.9.0 - /opt/homebrew/bin/node
  Yarn: 1.22.21 - /opt/homebrew/bin/yarn
  npm: 10.8.3 - /opt/homebrew/bin/npm
  pnpm: 9.5.0 - /opt/homebrew/bin/pnpm
  bun: 1.1.26 - ~/.bun/bin/bun
Browsers:
  Chrome: 131.0.6778.86
  Safari: 17.1
npmPackages:
  @faker-js/faker: ^9.2.0 => 9.2.0

Which module system do you use?

  • CJS
  • ESM

Used Package Manager

npm

@sounmind sounmind added c: bug Something isn't working s: pending triage Pending Triage labels Nov 25, 2024
@xDivisionByZerox
Copy link
Member

I read the documentation and thought that faker.phone.number() would return a string of the form xxx-xxx-xxxx, but it doesn't.

The documentation for the default case style = 'human' is pretty clear IMO:

'human': (default) A human-input phone number, e.g. "555-770-7727" or "555.770.7727 x1234"

The "e.g." means that these are example values. So the function can return many different styles which are all considered human readable. We specifically designed this to return different (but common) styles. That way your system can be tested for potential edge cases.

If you need THIS specific pattern, you are free to write your own method:

function phoneNumber() {
  return `${faker.string.numeric(3)}-${faker.string.numeric(3)}-${faker.string.numeric(4)}`;
}

@xDivisionByZerox xDivisionByZerox added has workaround Workaround provided or linked and removed s: pending triage Pending Triage labels Nov 25, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Nov 25, 2024

I read the documentation and thought that faker.phone.number() would return a string of the form xxx-xxx-xxxx, but it doesn't.

Are you referring to the examples?

faker.phone.number() // '961-770-7727'
faker.phone.number({ style: 'human' }) // '555.770.7727 x1234'
faker.phone.number({ style: 'national' }) // '(961) 770-7727'
faker.phone.number({ style: 'international' }) // '+15551234567'

Currently, the examples only show one example per invocation, as it isn't feasible to provide one for every possible variant, as there might be 10+ per layer of patterns stacking up to millions of combinations per locale.

e.g. faker.food.description has 20 patterns with ~4 placeholders each resulting in ~270 Mio possible result values.
Even with only the top level patterns for human "phone numbers", 16 examples are too much to display.
Please note that even if we put all those examples there, this would only cover the English locale.

If you want to get a feeling on what values to expect, then our website overs the ability to execute faker functions in the browser console (or you can do so locally).

Need to supplement the documentation so that readers can fully expect the value returned by faker.phone.number().

Do you have a suggestion on how to do that?

@ST-DDT ST-DDT added s: awaiting more info Additional information are requested m: phone Something is referring to the phone module p: 1-normal Nothing urgent c: docs Improvements or additions to documentation labels Nov 25, 2024
@sounmind
Copy link
Author

sounmind commented Nov 26, 2024

@ST-DDT I understand that you can't include every example type in your documentation, but can we at least imply that it's not just one type? Someone unfamiliar with fakers might have a hard time predicting based on this documentation alone.

Perhaps if I had read the documentation carefully from the beginning, I wouldn't have been so misled. I'm usually the type of person who checks examples first. Sorry if my comment came off as a bit harsh!

I wanted to highlight that “human” is the default for faker.phone.number(). This would guide readers to learn about the “human” style. It would also help them see that the function returns more than one type of value.

I just thought it would prevent someone like me in the future from getting confused. and if you disagree, feel free to close the PR!

@ST-DDT ST-DDT added s: needs decision Needs team/maintainer decision and removed s: awaiting more info Additional information are requested labels Nov 26, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Nov 26, 2024

Thanks for your feedback ❤️ . We will talk about it in the next team meeting.

@sounmind
Copy link
Author

sounmind commented Nov 27, 2024

Close with #3284. Please see the PR I mentioned to see more detailed discussion.

@ST-DDT @xDivisionByZerox Thank you for your kind feedback and It was a pleasure to be a part of this conversation. Hope we can discuss another contribution in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working c: docs Improvements or additions to documentation has workaround Workaround provided or linked m: phone Something is referring to the phone module p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants