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: add @since tags to all methods #1337

Merged
merged 10 commits into from
Sep 8, 2022

Conversation

matthewmayer
Copy link
Contributor

Following on from discussion in #1241 i added @since jsdoc tags to all existing public methods, and exposed this in the documentation:

image

This is a one-off large diff for this initial integration, in future it would just be necessary to add @since tags for any newly added methods.

@matthewmayer matthewmayer requested a review from a team September 6, 2022 18:34
@matthewmayer matthewmayer requested a review from a team as a code owner September 6, 2022 18:34
@xDivisionByZerox xDivisionByZerox added c: docs Improvements or additions to documentation p: 1-normal Nothing urgent labels Sep 6, 2022
@xDivisionByZerox xDivisionByZerox added this to the v7 - Current Major milestone Sep 6, 2022
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

I think I like this.

Please also add a new test case to: https://github.com/faker-js/faker/blob/main/test/scripts/apidoc/signature.example.ts

Also not sure whether there should be an empty line after the @examples and before @since.

scripts/apidoc/utils.ts Outdated Show resolved Hide resolved
src/modules/color/index.ts Outdated Show resolved Hide resolved
@xDivisionByZerox
Copy link
Member

Also not sure whether there should be an empty line after the @examples and before @since.

There definitely should be IMO. Looks a lot better with all topics grouped. Much easier to scan (as a human) as well.

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.

Please add spaces around the @since tag so it aligns with the other tags.

@matthewmayer
Copy link
Contributor Author

I think I like this.

Please also add a new test case to: https://github.com/faker-js/faker/blob/main/test/scripts/apidoc/signature.example.ts

Also not sure whether there should be an empty line after the @examples and before @since.

Sorry my typescript is not really up to this. Maybe someone else can help me with the tests

@matthewmayer
Copy link
Contributor Author

added the newlines and made the other suggested changes.

Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

I will address the test later today.

src/modules/address/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Really cool 👍
Count me as approved when all the missing requested/required tasks are finished

src/modules/address/index.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #1337 (3235c75) into main (508082c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 3235c75 differs from pull request most recent head 5fbc511. Consider uploading reports for the commit 5fbc511 to get more accurate results

@@           Coverage Diff            @@
##             main    #1337    +/-   ##
========================================
  Coverage   99.63%   99.63%            
========================================
  Files        2163     2163            
  Lines      240701   241242   +541     
  Branches     1017     1018     +1     
========================================
+ Hits       239817   240359   +542     
+ Misses        863      862     -1     
  Partials       21       21            
Impacted Files Coverage Δ
src/modules/address/index.ts 99.82% <100.00%> (+0.01%) ⬆️
src/modules/animal/index.ts 100.00% <100.00%> (ø)
src/modules/color/index.ts 99.73% <100.00%> (+0.02%) ⬆️
src/modules/commerce/index.ts 100.00% <100.00%> (ø)
src/modules/company/index.ts 100.00% <100.00%> (ø)
src/modules/database/index.ts 100.00% <100.00%> (ø)
src/modules/datatype/index.ts 96.24% <100.00%> (+0.21%) ⬆️
src/modules/date/index.ts 99.10% <100.00%> (+0.05%) ⬆️
src/modules/fake/index.ts 100.00% <100.00%> (ø)
src/modules/finance/index.ts 100.00% <100.00%> (+0.23%) ⬆️
... and 17 more

Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

faker.fake and faker.unique are missing the @since tags.
The @since tags won't be picked up for the color module.
I'm not sure why.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 7, 2022

The color methods have multiple signatures. You have to add the @since either the last one (with jsdocs) or all of them.

@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Sep 7, 2022
@matthewmayer
Copy link
Contributor Author

In general it seems its only reading the JSDoc for the last version of a method with multiple signatures like faker.color.rgb()? So in general should the since tag be only added to the last one, or all of them?

@ST-DDT
Copy link
Member

ST-DDT commented Sep 8, 2022

I think the last one is enough.

@Shinigami92
Copy link
Member

Depends on whether you want to show this to the user who is currently using faker in an IDE or if you only want to annotate this data to generate better fakerjs.dev docs.

If first, then you need to add it to all JSDocs

@Shinigami92
Copy link
Member

Current test results in CI:
image

@matthewmayer
Copy link
Contributor Author

i agree maybe all is better. will manually fix the ones in color

@matthewmayer
Copy link
Contributor Author

@Shinigami92 how can i run those tests locally to make sure I caught everything?

@Shinigami92
Copy link
Member

@Shinigami92 how can i run those tests locally to make sure I caught everything?

As easy as: pnpm run test 👀

@matthewmayer
Copy link
Contributor Author

thanks, tests now run green

Shinigami92
Shinigami92 previously approved these changes Sep 8, 2022
src/modules/color/index.ts Outdated Show resolved Hide resolved
src/modules/color/index.ts Outdated Show resolved Hide resolved
@matthewmayer
Copy link
Contributor Author

fixed those two issues

@ST-DDT ST-DDT requested review from a team September 8, 2022 09:54
@ST-DDT ST-DDT mentioned this pull request Sep 8, 2022
@ST-DDT ST-DDT changed the title docs: add since tags to all public methods, show available since in docs docs: add @since tags to all methods Sep 8, 2022
@ST-DDT ST-DDT enabled auto-merge (squash) September 8, 2022 16:36
@ST-DDT ST-DDT merged commit 1fe2877 into faker-js:main Sep 8, 2022
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 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.

5 participants