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

docs(phone): update faker.phone.number() example #3284

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

sounmind
Copy link

Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 8de5ce0
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/674537e008a2670008d9eeff
😎 Deploy Preview https://deploy-preview-3284.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sounmind sounmind marked this pull request as ready for review November 26, 2024 02:49
@sounmind sounmind requested a review from a team as a code owner November 26, 2024 02:49
@sounmind sounmind changed the title Update faker.phone.number() document docs: faker.phone.number() Nov 26, 2024
@sounmind sounmind changed the title docs: faker.phone.number() docs(phone): update faker.phone.number() example Nov 26, 2024
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (b390432) to head (8de5ce0).

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #3284   +/-   ##
=======================================
  Coverage   99.96%   99.97%           
=======================================
  Files        2806     2806           
  Lines      217157   217157           
  Branches      982      977    -5     
=======================================
+ Hits       217088   217093    +5     
+ Misses         69       64    -5     
Files with missing lines Coverage Δ
src/modules/phone/index.ts 90.90% <ø> (ø)

... and 1 file with indirect coverage changes

@ST-DDT ST-DDT added c: docs Improvements or additions to documentation p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision m: phone Something is referring to the phone module labels Nov 26, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Nov 26, 2024
@@ -19,7 +19,7 @@ export class PhoneModule extends ModuleBase {
* @see faker.helpers.fromRegExp(): For generating a phone number matching a regular expression.
*
* @example
* faker.phone.number() // '961-770-7727'
* faker.phone.number() // '961-770-7727' (default 'human' style)
Copy link
Member

@xDivisionByZerox xDivisionByZerox Nov 26, 2024

Choose a reason for hiding this comment

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

This would require a change on ALL our methods for consistency purposes. Also, what happens when the options object get more parameters in the future? Do you intend to list all default values for each parameter here? Take a look at one of the potential worst cases: internet.jwt(). Would you honestly make the call to add the following chang eto the example?

     * @example
-    * faker.internet.jwt() // 'eyJhbGciOiJIUzI1NiJ9.eyJpYXQiOjE3MTg2MTM3MTIsImV4cCI6MTcxODYzMzY3OSwibmJmIjoxNjk3MjYzNjMwLCJpc3MiOiJEb3lsZSBhbmQgU29ucyIsInN1YiI6IjYxYWRkYWFmLWY4MjktNDkzZS1iNTI1LTJjMGJkNjkzOTdjNyIsImF1ZCI6IjczNjcyMjVjLWIwMWMtNGE1My1hYzQyLTYwOWJkZmI1MzBiOCIsImp0aSI6IjU2Y2ZkZjAxLWRhMzMtNGUxNi04MzJiLTFlYTk3ZGY1MTQ2YSJ9.5iUgaCaFVPZ8d1QD0xMjoeJbmPVyUfKfoRQ6Njzm5MLp5F4UMh5REbPCrW70fAkr'
+    * faker.internet.jwt() // 'eyJhbGciOiJIUzI1NiJ9.eyJpYXQiOjE3MTg2MTM3MTIsImV4cCI6MTcxODYzMzY3OSwibmJmIjoxNjk3MjYzNjMwLCJpc3MiOiJEb3lsZSBhbmQgU29ucyIsInN1YiI6IjYxYWRkYWFmLWY4MjktNDkzZS1iNTI1LTJjMGJkNjkzOTdjNyIsImF1ZCI6IjczNjcyMjVjLWIwMWMtNGE1My1hYzQyLTYwOWJkZmI1MzBiOCIsImp0aSI6IjU2Y2ZkZjAxLWRhMzMtNGUxNi04MzJiLTFlYTk3ZGY1MTQ2YSJ9.5iUgaCaFVPZ8d1QD0xMjoeJbmPVyUfKfoRQ6Njzm5MLp5F4UMh5REbPCrW70fAkr' (default { alg: faker.internet.jwtAlgorithm(), typ: 'JWT' } header, { iat: faker.date.recent(), exp: faker.date.soon(), nbf: faker.date.anytime(), iss: faker.company.name(), sub: faker.string.uuid(), aud: faker.string.uuid(), jti: faker.string.uuid() } payload, faker.defaultRefDate() refDate )

The formatting of line alone is a headache in itself.

Line 33 perfectly describes describes the default behaviour of the function. There is nothing to change here IMO.

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

Please also note the definition of the word example:

one of a number of things, or a part of something, taken to show the character of the whole:
This painting is an example of his early work.

Synonyms: specimen, sample
[...]

Source: dictionary.com


Although, I have to thank you. By writing this comment I noticed that the default example for internet.jwt was missing it's return value:

* faker.internet.jwt()

Copy link
Author

@sounmind sounmind Nov 26, 2024

Choose a reason for hiding this comment

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

I overlooked the potential impact on document consistency.
I also didn’t realize there could be examples like JWT.
The definition of “example” was helpful as well.

Thank you for explaining with rich context!
I now understand the current way the document is expressed.

There aren’t many people like me giving similar feedback, so I think it’s better not to make this change. I'll close this PR soon.
I’m really glad, though, that this feedback incidentally helped catch the mistake regarding the jwt return value!

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we just had two lines with two different examples.

Copy link
Member

Choose a reason for hiding this comment

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

There aren’t many people like me giving similar feedback, so I think it’s better not to make this change. I'll close this PR soon.

It's unfortunate because we require feedback like this to understand the community's needs better. While we can't accept every request or make every change suggested by community members, such insights are invaluable for planning the project's future.

Please don't feel discouraged by the rejection of this particular change. I hope to see your contributions (in any size and form) in the future.

What if we just had two lines with two different examples.

I believe this would lead to inconsistencies in the documentation as well.

If I had to identify the root cause of this issue, I would say that the example value for the default seems "too perfect". How about we change it to another human-styled phone number that isn't in the form of xxx-xxx-xxxx?

Copy link
Member

@ST-DDT ST-DDT Nov 26, 2024

Choose a reason for hiding this comment

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

I also considered adding a refresh button to the examples replacing the values in the // comments.

Copy link
Member

Choose a reason for hiding this comment

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

I also considered adding a refresh button to the examples replacing the values in the // comments.

This sounds like super cool idea! Could you extract that idea into it own issue? That way we can start dedicated discussions, make arguments and collect implementation ideas in its own context.

Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: docs Improvements or additions to documentation 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 this pull request may close these issues.

The faker.phone.number() is returned differently than in the documentation.
4 participants