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: improve docs for faker.number.float #2607

Merged
merged 7 commits into from
Jan 19, 2024

Conversation

matthewmayer
Copy link
Contributor

@matthewmayer matthewmayer commented Jan 19, 2024

cleanup of docs after #1855 and #2564

Make it clearer that precision is a deprecated version of multipleOf, and that you should only pass one of {precision, multipleOf, fractionDigits}. Fix an example. Move information about inclusive/exclusive to the min/max parameters.

@matthewmayer matthewmayer requested a review from a team January 19, 2024 07:17
@matthewmayer matthewmayer requested a review from a team as a code owner January 19, 2024 07:17
Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (953087c) 99.57% compared to head (6e4fb14) 99.57%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2607   +/-   ##
=======================================
  Coverage   99.57%   99.57%           
=======================================
  Files        2807     2807           
  Lines      250448   250441    -7     
  Branches     1148     1151    +3     
=======================================
+ Hits       249374   249377    +3     
+ Misses       1046     1036   -10     
  Partials       28       28           
Files Coverage Δ
src/modules/number/index.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added c: docs Improvements or additions to documentation p: 1-normal Nothing urgent m: number Something is referring to the number module labels Jan 19, 2024
@ST-DDT ST-DDT mentioned this pull request Jan 19, 2024
src/modules/number/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added this to the v8.x milestone Jan 19, 2024
@ST-DDT ST-DDT requested review from a team January 19, 2024 10:05
@xDivisionByZerox
Copy link
Member

General question: Do we want to write JSDocs in one line? In our Markdown files we generally break after certain keypoints (end of sentence, punctuation, reaching character line limit). I was having trouble reading these long description lines on my phone. Might not be that hard to read on a bigger screen, but I generally prefer breaking the line, at least after the end of a sentence. I'm I the only one? What's your take on this?

@Shinigami92
Copy link
Member

General question: Do we want to write JSDocs in one line? In our Markdown files we generally break after certain keypoints (end of sentence, punctuation, reaching character line limit). I was having trouble reading these long description lines on my phone. Might not be that hard to read on a bigger screen, but I generally prefer breaking the line, at least after the end of a sentence. I'm I the only one? What's your take on this?

Thanks for raising this, now I feel confirmed as I encountered this as well but did not say anything

@matthewmayer
Copy link
Contributor Author

Ideally we could have a lint or prettier rule to do this automatically as I'm usually on a desktop with word wrap enabled when editing, so it's not something I typically think about.

@Shinigami92
Copy link
Member

Ideally we could have a lint or prettier rule to do this automatically as I'm usually on a desktop with word wrap enabled when editing, so it's not something I typically think about.

Either eslint-plugin-jsdoc has something for this already and needs to be configured, or as I suggest: do not use the IDE Editor Panel but hover over the function and investigate how it is rendered in the IDE tooltip.

@matthewmayer
Copy link
Contributor Author

Ideally we could have a lint or prettier rule to do this automatically as I'm usually on a desktop with word wrap enabled when editing, so it's not something I typically think about.

Either eslint-plugin-jsdoc has something for this already and needs to be configured, or as I suggest: do not use the IDE Editor Panel but hover over the function and investigate how it is rendered in the IDE tooltip.

single newlines in Markdown have no effect on how the text is rendered. So this is only important when viewing the raw markdown.

@Shinigami92
Copy link
Member

Ideally we could have a lint or prettier rule to do this automatically as I'm usually on a desktop with word wrap enabled when editing, so it's not something I typically think about.

Either eslint-plugin-jsdoc has something for this already and needs to be configured, or as I suggest: do not use the IDE Editor Panel but hover over the function and investigate how it is rendered in the IDE tooltip.

single newlines in Markdown have no effect on how the text is rendered. So this is only important when viewing the raw markdown.

No, what we mean is the preference of:

Returns a single random floating-point number, by default between `0.0` and `1.0`.

To change the range, pass a `min` and `max` value. To limit the number of decimal places, pass a `multipleOf` or `fractionDigits` parameter.

instead of

Returns a single random floating-point number, by default between `0.0` and `1.0`. To change the range, pass a `min` and `max` value. To limit the number of decimal places, pass a `multipleOf` or `fractionDigits` parameter.

Please recognize the blank line between the sentences, because this results in a real line break in the rendered tooltip.

@matthewmayer
Copy link
Contributor Author

Ideally we could have a lint or prettier rule to do this automatically as I'm usually on a desktop with word wrap enabled when editing, so it's not something I typically think about.

Either eslint-plugin-jsdoc has something for this already and needs to be configured, or as I suggest: do not use the IDE Editor Panel but hover over the function and investigate how it is rendered in the IDE tooltip.

single newlines in Markdown have no effect on how the text is rendered. So this is only important when viewing the raw markdown.

No, what we mean is the preference of:

Returns a single random floating-point number, by default between `0.0` and `1.0`.

To change the range, pass a `min` and `max` value. To limit the number of decimal places, pass a `multipleOf` or `fractionDigits` parameter.

instead of

Returns a single random floating-point number, by default between `0.0` and `1.0`. To change the range, pass a `min` and `max` value. To limit the number of decimal places, pass a `multipleOf` or `fractionDigits` parameter.

Please recognize the blank line between the sentences, because this results in a real line break in the rendered tooltip.

its not terribly consistent at the moment. Most descriptions of methods are very terse. Where there are multiple sentences, sometimes linebreaks are used and sometimes they are not

https://fakerjs.dev/api/internet.html#username
https://fakerjs.dev/api/finance.html#ethereumaddress

@Shinigami92
Copy link
Member

You can decide, I approved already 🤷

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.

I'll approve as well. We should have some standards/rules to follow regarding comment format. Since we don't have that, I'll not block this PR with my question.

@xDivisionByZerox xDivisionByZerox merged commit 5991930 into faker-js:next Jan 19, 2024
20 checks passed
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: number Something is referring to the number module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants