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

Documentation: Generate Docs for the data module #7264

Merged
merged 3 commits into from Jun 19, 2018

Conversation

Projects
None yet
5 participants
@youknowriad
Contributor

youknowriad commented Jun 11, 2018

This PR adds docs generation for the data module's selectors/actions.

  • The docs live in docs/data/*.md
  • To regenerate the docs, we run npm run docs:generate-data-reference
  • It doesn't support the dynamic selectors/actions from the core-data module.
  • The output is printed as markdown.

It doesn't integrate with the Handbook yet but it should be possible to add a step to call this command line before doing the synchronization cc @pento

Its output is markdown to ease integration into other docs systems like the handbook.

The docs can be gitignored at some point but I think it's fine to do the generation manually as a start.

@youknowriad youknowriad self-assigned this Jun 11, 2018

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Jun 11, 2018

node.type === 'ExportNamedDeclaration' &&
node.declaration.type === 'FunctionDeclaration' &&
node.leadingComments &&
node.leadingComments.lenght !== 0

This comment has been minimized.

@Soean

Soean Jun 11, 2018

Member

.length

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 12, 2018

OMG yes ❤️ so much for this. I'll have a look through and see if they make sense to my relatively green self.

@tofumatt

I think this is great and there's some helpful stuff in here, but we should work to improve the signal-to-noise ratio of the docs and assume a lot less knowledge in them.

There are a lot of functions here whose name/name+parameters explain everything (or where if they don't, the docs are just re-stated function signatures.

I made some suggestions on particular docs but they apply throughout, really. If my suggestions make sense and can be extrapolated to the rest of the docs I'm happy to review again after a pass with my thoughts in mind.

If I need to clarify my general thoughts more/my criticisms are unclear just let me know 😄

This is an awesome effort and will be really handy for new devs when iterated upon. 👍

### getBlockType
Returns a block type by name.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

"type by name" is a bit awkward, especially because "name" isn't really a type that I'd expect to provide clarification.

This might be better stated as "Return a block type as a string (eg. by name)." An example would be even better.

### getCategories
Returns all the available categories.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

I've noticed "returns" instead of "return" used everywhere here and I'm not sure it makes sense in prose. I guess it's not wrong per-se but it doesn't really add anything 😄

It makes sense as a header but not in prose to me. I guess I like "Return all available categories" over "Returns all the available categories" but maybe that's also my English-as-a-first-language creeping through; we leave out articles like "the" a lot I find.

*Parameters*
* state: Data state.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

Is there any way we can make this an include in Markdown? It's repeated a lot. "redux state" might also be a bit better to use as a descriptor as well, for what it's worth.

### getDefaultBlockName
Returns the name of the default block name.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

The default block name in what context? I'm guessing it's for the editor context (and this is probably <p> by default), but that isn't clear from just reading the docs.

### getFallbackBlockName
Returns the name of the fallback block name.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

A lot of these functions are somewhat clear based on their function names, so an explanation like this one doesn't really help, I think.

But the docs could really shine if this explained (not in detail, but even just in passing), what a fallback block was. Why you'd use this and what it's for would really help contextualise a lot of these selector functions.

*Returns*
Whether the editor sidebar is opened.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

That is what it does, but what it returns is `true` if sidebar is open; `false` if sidebar is not open.

This comment has been minimized.

@aduth

aduth Jun 12, 2018

Member

That is what it does, but what it returns is `true` if sidebar is open; `false` if sidebar is not open.

Aside: Typically the primary description would include a "true if ..., otherwise false". Also there is type information available in the JSDoc {boolean} we are not including here.

*Returns*
Whether the plugin sidebar is opened.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

A lot of these have explanations that should be the return value and return values that better explain the function. Maybe it's worth switching them?

### getPreference

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

All the empty newlines should be cleaned up.

*Returns*
Preferences Object.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

Could we link to this? Otherwise this isn't a helpful explainer.

### initializeMetaBoxState
Returns an action object used to check the state of meta boxes at a location.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

This is an example of where the extra detail and explanation really helps. A lot of the docs around these selectors is stating the obvious or just re-stating their names and function method in a more verbose and manual way.

Maybe it's worth not adding redundant docs for a lot of them if it's very obvious.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 12, 2018

Thanks for the review @tofumatt Great feedback here.

I'd like to keep this PR focused on the docs generation aspect and not the descriptions/docs them selves. I'd like to tackle those comments in a separate PR (Probably worth creating an issue with a link to these comments).

The extra new lines is probably something related to the docs generation tool itself though.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 12, 2018

@tofumatt I created this issue to address these concerns about the descriptions etc... #7287 Feel free to update it

const code = fs.readFileSync( file, 'utf8' );
const parsedCode = espree.parse( code, {
attachComment: true,
ecmaVersion: 9,

This comment has been minimized.

@aduth

aduth Jun 12, 2018

Member

The downside here is we're not strictly following the Babel configuration, which may only become an issue for more bleeding edge functionality.

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

Maybe a comment pointing that out would be good; even if just so we can grep for "Babel" later and see why the docs aren't using our Babel config? 🤷‍♂️

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 12, 2018

So my bad, I didn't realise this was all generated; I should have realised with such a massive green line count 😆

Thanks for filing the issue related to the content of the documentation itself; for now my issues with this PR are just with nitpicks around the newlines and such 👍

@youknowriad youknowriad requested a review from tofumatt Jun 19, 2018

@tofumatt

r+wc (That's Mozilla-speak for approved-once-comments-are-addressed)

Code looks good and the generated documentation looks a bit cleaner too 👍

namespaces: {
core: {
title: 'WordPress Core Data',
// Figures out a way to generate docs for the dynamic actions/selectors

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

The tense here implies that the following code "figures out a way" to do something, but I'm thinking this is actually a TODO, eg:

// TODO: Figure out a way to generate docs for dynamic actions/selectors

Sounds like something best filed as a "code quality" issue 😆, but I'd at least tidy up this comment so it isn't confusing.

actions: [ path.resolve( root, 'blocks/store/actions.js' ) ],
},
'core/editor': {
title: 'The Editor\'s Data',

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

Does the style guide not let us use double quotes rather than escaping? It's sooooo ugly.

This comment has been minimized.

@gziolo

gziolo Jun 19, 2018

Member

It looks like this is how things are set up.

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

Filed #7366, then 😄

actions: [ path.resolve( root, 'viewport/store/actions.js' ) ],
},
'core/nux': {
title: 'The NUX module Data',

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

If you wanted to add "(New User Experience)" after "NUX" that might be handy as readers of these docs could be less experienced with that acronym. 😄

].join( '\n' );
}
module.exports = function( parsed, rootFolder ) {

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

parsed what? I think it's parsedCode, right? Could we label it a bit better?

const code = fs.readFileSync( file, 'utf8' );
const parsedCode = espree.parse( code, {
attachComment: true,
ecmaVersion: 9,

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

Maybe a comment pointing that out would be good; even if just so we can grep for "Babel" later and see why the docs aren't using our Babel config? 🤷‍♂️

@@ -171,6 +173,7 @@
"test-unit:coverage-ci": "npm run test-unit -- --coverage --maxWorkers 1 && codecov",
"test-unit:watch": "npm run test-unit -- --watch",
"test-unit-php": "docker-compose run --rm wordpress_phpunit phpunit",
"test-unit-php-multisite": "docker-compose run -e WP_MULTISITE=1 --rm wordpress_phpunit phpunit"
"test-unit-php-multisite": "docker-compose run -e WP_MULTISITE=1 --rm wordpress_phpunit phpunit",
"docs:generate-data-reference": "node docs/data/tool"

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

Can you move this above, under the dev: commands? The rest of these commands are sorted alphabetically so that'll make them easier to scan.

This comment has been minimized.

@gziolo

gziolo Jun 19, 2018

Member

Thanks, I like to have things sorted alphabetically. It's much easier to find what you need.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 19, 2018

Yes, let's move on with this one and iterate later. We should discuss it at Core JS chat together with @atimmer so we could align with the parallel efforts for the existing WordPress codebase: https://make.wordpress.org/core/2018/06/14/js-docs-initiative-add-inline-docs-for-javascript-part-2/.

By the way, @atimmer, your review is warmly welcomed :)

@youknowriad youknowriad merged commit 3bbb260 into master Jun 19, 2018

2 checks passed

codecov/project 46.56% remains the same compared to 9aeccef
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/automate-data-module-docs branch Jun 19, 2018

@youknowriad youknowriad added this to the 3.1 milestone Jun 19, 2018

parsedCode.body.forEach( ( node ) => {
if (
node.type === 'ExportNamedDeclaration' &&
node.declaration.type === 'FunctionDeclaration' &&

This comment has been minimized.

@aduth

aduth Sep 10, 2018

Member

FYI this excludes memoized selectors. See fix/improvement at #9756

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment