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

Add support for the @see tag #236

Merged
merged 9 commits into from
May 20, 2020
Merged

Add support for the @see tag #236

merged 9 commits into from
May 20, 2020

Conversation

0xCLARITY
Copy link
Contributor

We use eslint-plugin-tsdoc in some of our projects, but we also use @see tags. I would like to be able to keep using both of these. This PR would address #235.

This is blatantly stolen from #205. @rbuckton , if you have any feedback, please let me know.

The 205 PR claims that implementing @see tag is blocked on adding support for synonyms, but in the JSDoc @see tag documentation it does not have a synonym listed. So, I thought we would be able to implement @see tag functionality without building synonyms.

I'm a little in over my head here, but I'm happy to learn.

@msftclas
Copy link

msftclas commented May 8, 2020

CLA assistant check
All CLA requirements met.

@Gerrit0
Copy link
Contributor

Gerrit0 commented May 8, 2020

Both of these will link to the bar function.
@see {@link bar}
@see bar

This seems to go against TSDoc's philosophy of one obvious way. I prefer the first since it seems reasonable to want to write @see {@link bar} for why the world is round.

@rbuckton
Copy link
Member

rbuckton commented May 8, 2020

I already have a PR to address this: #205

@rbuckton
Copy link
Member

rbuckton commented May 8, 2020

One of the reasons to support both forms is to support existing documentation when someone is migrating from JavaScript to TypeScript.

@0xCLARITY
Copy link
Contributor Author

I already have a PR to address this: #205

Definitely didn't mean to step on your toes, and this PR is mostly just copy-pasting some relevant stuff from your PR.

My reasoning for creating a new PR is that #205 appears to be blocked on implementing synonyms in TSDoc, but looking at the JSDoc @see tag documentation I don't see any synonyms listed for the @see tag. Am I missing something?

@rbuckton
Copy link
Member

rbuckton commented May 8, 2020

You're not missing anything. seealso mostly comes from XML doc comments in C#. You're correct that synonyms aren't strictly necessary for this feature, but I still believe supporting both @see {@link foo} ... and @see foo ... are useful for anyone coming from using the jsdoc app to generate their documentation. If tsdoc would rather its users choose a single syntax, it might be feasible to still support @see foo ... with a warning (which could be disabled by tools like api-extractor, etc.).

@octogonz
Copy link
Collaborator

octogonz commented May 8, 2020

I added some comments in #235

Let's agree on the spec before we merge a PR.

@octogonz
Copy link
Collaborator

@hbergren Based on the thread in #235 I've proposed some changes in PR https://github.com/hbergren/tsdoc/pull/1 into your branch.

Could you promote this PR to non-draft so we can get it merged? @see seems like an easy addition and a valuable feature.

/**
* Append an item to the seeBlocks collection.
*/
public appendSeeBlock(block: DocBlock): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm... this API feels weird to me. The original design is that the "core" tags (Standardization.Core) would get dedicated properties in the DocComment API, and then all other tags would go into the customBlocks array.

But this has a downside that any code relying on customBlocks will get broken by an update that promotes a block to being "core". And we seem to need helpers like appendSeeBlock() for each property.

Perhaps a better design would be to deprecate customBlocks and replace it with a collection blocks that simply includes ALL the blocks. Then helpers like seeBlocks would be a shorthand for something like this:

  public getBlocksOfType(tag: TSDocTagDefinition): ReadonlyArray<DocBlock> {
    return this._blocks.filter(x => x.blockTag.tagNameWithUpperCase === tag.tagNameWithUpperCase);
  }

  public get seeBlocks(): ReadonlyArray<DocBlock> {
    return this.getBlocksOfType(StandardTags.see);
  }

@hbergren Do you like this design better? If so we can implement it in a separate PR.

Copy link
Collaborator

@octogonz octogonz May 10, 2020

Choose a reason for hiding this comment

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

Looking more closely at the code, it seems that customBlocks is also used by TSDocEmitter when rearranging the blocks into a normalized order. The summary/remarks/params/returns are ordered first, then all the "custom blocks" can be emitted afterwards. I feel like we can find a better design for that, which avoids this problem of breaking the API whenever we promote a tag to be standardized.

But fixing that problem should probably be tackled separately from introducing DocComment.blocks to support the improvement proposed above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... this API feels weird to me. Perhaps a better design would be to deprecate customBlocks and replace it with a collection blocks that simply includes ALL the blocks.

I think your proposed design is pretty solid. I'm happy to help implement that in a future PR.

How would that potentially work with TSDocEmitter? We ensure that the this._blocks property is ordered before it gets to TSDocEmitter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's address this issue in a separate PR. When I started to prototype it, it has some design questions of its own. I am okay with temporarily introducing appendSeeBlock() to get this PR merged. We can mark the API as @internal for now.

@0xCLARITY
Copy link
Contributor Author

Based on the thread in #235 I've proposed some changes in PR hbergren#1 into your branch. Could you promote this PR to non-draft so we can get it merged? @see seems like an easy addition and a valuable feature.

Approved that PR with one question just to double-check my understanding.

@0xCLARITY
Copy link
Contributor Author

@octogonz, I approved and merged your PR for see-tag. Let me know what else I can do to help move this forward.

@octogonz octogonz merged commit 2a6bd2e into microsoft:master May 20, 2020
@octogonz
Copy link
Collaborator

@hbergen Thanks again for making this PR! It was published as TSDoc 0.12.20.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants