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

data: set up auto-generated API docs #14277

Merged
merged 9 commits into from
Mar 27, 2019
Merged

data: set up auto-generated API docs #14277

merged 9 commits into from
Mar 27, 2019

Conversation

oandregal
Copy link
Member

Related to #14227

This PR sets up auto-generated API docs for the data package through the npm run docs:build command and updates its README file.

Test instructions

  • Delete or modify the contents at packages/data/README.md within the start <!-- START TOKEN(Autogenerated API docs) --> and end <!-- END TOKEN(Autogenerated API docs) --> tokens.
  • Execute npm run docs:build and verify they are re-created.
  • Review that the API docs match the JSDoc of the exported symbols.

@oandregal oandregal self-assigned this Mar 6, 2019
@oandregal oandregal added [Type] Developer Documentation Documentation for developers [Package] Data /packages/data Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels Mar 6, 2019
@oandregal oandregal added this to the 5.3 (Gutenberg) milestone Mar 6, 2019
@oandregal oandregal force-pushed the update/api-docs-data branch 3 times, most recently from 0ca167f to 7ce0e70 Compare March 8, 2019 07:35
@gziolo gziolo removed the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Mar 8, 2019
@gziolo
Copy link
Member

gziolo commented Mar 8, 2019

I wouldn't consider it as a Good First Review. It should be reviewed by @aduth, @youknowriad or @coderkevin :)

@oandregal oandregal requested a review from gziolo March 8, 2019 11:31

[src/index.js#L14-L14](src/index.js#L14-L14)

Undocumented declaration.
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs documenting. To be added as a JSDoc comment in the component.

Copy link
Member

@aduth aduth Mar 11, 2019

Choose a reason for hiding this comment

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

This needs documenting. To be added as a JSDoc comment in the component.

Are you considering this a blocker? Do you need help with it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a blocker from my point of view. It was undocumented and will stay that way after all.

I feel this is ready and is a good addition, but would like feedback on 1) did I miss something? 2) does the general approach works well for this package or are things we should improve?


[src/index.js#L14-L14](src/index.js#L14-L14)

Undocumented declaration.
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs documenting. To be added as a JSDoc comment in the component.

@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I'm a bit more sensitive to this document than most other modules, since it's a key integration point for a majority of plugins. That said, I think while there's some general reorganization, it appears most of the examples are retained. Generally we might want to push toward more tutorials as part of the handbook, anyways.

packages/data/README.md Outdated Show resolved Hide resolved
packages/data/src/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about the arbitrary order of the documentation, but being that it's automated I understand it somewhat. It'd be nice if the api can be grouped a bit more. For example, general api functions that are used more frequently are registerStore, dispatch, select, subscribe, withSelect and withDispatch. Then the others tend to be for more advanced usage. With automation, could grouping be controlled a bit via special tags (i.e. @docs:advanced)?

packages/data/README.md Outdated Show resolved Hide resolved
@oandregal oandregal force-pushed the update/api-docs-data branch from afd9249 to 0bff374 Compare March 20, 2019 09:14
@oandregal
Copy link
Member Author

@aduth @nerrad Please, feel welcome to commit your suggestions to reorganization directly to this branch. I'm not familiar with this package as you are, so I'll need a little guidance.

Regarding grouping - for context, see related discussion. TLDR is:

  • we could switch to export order instead of alphabetical, but I don't think it'll provide better results. It takes a lot of energy to keep them well organized and up to date. As an example in point, these are the artifacts that aren't documented in this package: createRegistry, createRegistryControl, createRegistrySelector, plugins, RegistryConsumer, RegistryProvider, use, withRegistry.
  • API docs work best when optimized for lookup, not onboarding (ie: alphabetical over most-common used first). We should create better onboarding docs in the handbook.

@oandregal
Copy link
Member Author

With automation, could grouping be controlled a bit via special tags (i.e. @docs:advanced)?

@nerrad I wanted to address this separately because that's a great question.

We currently have a very raw mechanism for grouping: we can update different parts of the README leveraging the --ignore and --use-token options.

I don't think that's enough. I welcome new ideas and look forward to adding support for them! Some that I've already listed are: #14333, #14332 and #14292 Note that these are generic and decoupled from a specific output. I'd want us to avoid adding metadata that is tied to a specific output (for example: adding metadata about which section in the README a symbol should be embedded, etc.).

@oandregal oandregal requested review from nerrad and aduth March 20, 2019 10:17
@oandregal
Copy link
Member Author

@nerrad @aduth any chance you see this merged? I'd prefer to have these docs automated as automation is already showing how it helps to keep the docs up to date in the other packages. If you are strongly against this I'd rather close it so we can move on to other things.

Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

I say let's get this merged and then iterate. Documentation is something that can be improved on over time. I think there may be some benefit to getting this merged now being that there's some churn happening in wp.data that would be advantageous to get documented where possible.

@oandregal oandregal merged commit ba39b5d into master Mar 27, 2019
@oandregal oandregal deleted the update/api-docs-data branch March 27, 2019 08:31
@youknowriad youknowriad added this to the 5.4 (Gutenberg) milestone Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants