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

docgen: Omit docblocks with private tag #15173

Merged
merged 5 commits into from May 3, 2019

Conversation

@aduth
Copy link
Member

commented Apr 25, 2019

Closes #14294

This pull request seeks to enhance @wordpress/docgen to omit symbols which include a @private tag in their JSDoc. In turn, this was used to omit the reference to wp.blocks.children from the Blocks API documentation.

Open Questions:

There might be a question as to how or if we choose to omit deprecated APIs from the documented output, as in this case the API is technically deprecated. If we choose to omit this deprecated API, should we do the same for all (or should it be an option of docgen to omit deprecated APIs?). Otherwise, should this API not be marked as private and instead simply marked as deprecated, but allowed to be included in the documented output?

Testing Instructions:

Verify there are no changes from running npm run docs:build.

Ensure unit tests pass:

npm run test-unit
@@ -1,3 +1,16 @@
let _lastSum;

This comment has been minimized.

Copy link
@aduth

aduth Apr 25, 2019

Author Member

For what it's worth, these test fixtures aren't super valuable in their current form, since this could be totally wrong, and the tests still wouldn't fail, because the output isn't regenerated as part of the test, but presumably as a separate manual process.

Related: #13329 (comment)

This comment has been minimized.

Copy link
@nosolosw

nosolosw Apr 25, 2019

Member

I don't think this particular fixture is used anywhere. For Markdown we should add the test here.

This comment has been minimized.

Copy link
@aduth

aduth Apr 25, 2019

Author Member

I don't think this particular fixture is used anywhere. For Markdown we should add the test here.

But it's not the formatter which is responsible to exclude the private DocBlocks, it occurs in the engine.js filtering.

This comment has been minimized.

Copy link
@aduth

aduth Apr 25, 2019

Author Member

I don't think this particular fixture is used anywhere.

Should it be removed here then?

This comment has been minimized.

Copy link
@nosolosw

nosolosw Apr 26, 2019

Member

Should it be removed here then?

Yes.

But it's not the formatter which is responsible to exclude the private DocBlocks, it occurs in the engine.js filtering.

Oh, I see now, commented too quickly. The ignored symbols are actually filtered out by the CLI, not the formatter or the engine, so it's more tricky to test.

Filtering symbols (by name or tag) shouldn't be something the CLI takes care of. Ideally, we would push this code down to get-intermediate-representation.js. For the purpose of shipping this, I'm fine if we defer that refactor until later provided that we create an issue to track it.

Show resolved Hide resolved packages/docgen/coverage.md Outdated
@@ -1,4 +1,7 @@
const getTagsByName = ( tags, ...names ) => tags.filter( ( tag ) => names.some( ( name ) => name === tag.title ) );

This comment has been minimized.

Copy link
@nosolosw

nosolosw Apr 25, 2019

Member

great little extraction

@aduth aduth force-pushed the add/docgen-omit-private branch from 5cdb16b to 01d3776 Apr 26, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

Rebased to:

  • Resolve conflict
  • Squash a few commits
  • Remove the markdown fixtures files
@aduth

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

@nosolosw At your convenience: Do you have any other thoughts here? As I see it, it seems in good shape after changes stemming from your last round of feedback.

@nosolosw nosolosw self-requested a review May 3, 2019

@nosolosw
Copy link
Member

left a comment

Let's get this in, it's a great addition I personally wanted for some time. I'm merging into master myself as I'm working on something that will benefit from having this already present :)

@nosolosw nosolosw merged commit f582122 into master May 3, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@nosolosw nosolosw deleted the add/docgen-omit-private branch May 3, 2019

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.