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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/modules/phone/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

faker/src/modules/phone/index.ts

Line 33 in b390432

* - `'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/src/modules/internet/index.ts

Line 1101 in b390432

* faker.internet.jwt()

Copy link
Author

@sounmind sounmind Nov 26, 2024 •
edited
Loading

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 •
edited
Loading

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.

Done: #3287

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