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
Merged

Conversation

aduth
Copy link
Member

@aduth aduth 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;
Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Should it be removed here then?

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

great little extraction

@aduth
Copy link
Member Author

aduth commented Apr 26, 2019

Rebased to:

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

@aduth
Copy link
Member Author

aduth 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.

@oandregal oandregal self-requested a review May 3, 2019 16:14
Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

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 :)

@oandregal oandregal merged commit f582122 into master May 3, 2019
@oandregal oandregal deleted the add/docgen-omit-private branch May 3, 2019 16:16
@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
Labels
[Package] Docgen /packages/docgen [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docgen: ignore symbols with the private JSDoc tag
3 participants