FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Editor support for link tag by sandersn · Pull Request #41877 · microsoft/TypeScript · GitHub

/ TypeScript Public

Editor support for link tag - #41877

Merged
Nathan Shively-Sanders (sandersn) merged 58 commits into
masterfrom
editor-support-for-links
Mar 16, 2021
ConversationCommits (58)ChecksFiles changed
Merged

Editor support for link tag#41877
Nathan Shively-Sanders (sandersn) merged 58 commits into
masterfrom
editor-support-for-links

Conversation

Nathan Shively-Sanders (sandersn) commented Dec 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes #35524 (80% solution)

This PR makes the parser parse @link tag and the language service offer symbols for entity names found at the beginning of those tags. Links are stored redundantly with comment text, so they're only used to provide (1) a span to an editor to format in some way (2) a symbol for the declaration of the link's target.

Future PRs

  1. Parse # as type-space element access: Class#method. As well as, possibly, the normal TS syntax Class["method"], which doesn't currently parse either.

Open questions

  1. I parse @see {@link ...} as a @see with no reference and a link in its comment text. I think that's OK.
  2. I intentionally do not parse { @link X}, with a space separating { and @. I saw this only twice in my corpus, but Andrew Casey (@amcasey) points out that it's easy to support with an additional skipWhitespace call. I originally skipped it because I had slightly less lookahead when the next token after { was @, but that made the code unreadable, so I dropped it.

Resolved questions

  1. I introduced JSDocText and JSDocLink nodes into the AST, which better reflects the semantic relation of comments to their tags. It is more expensive, though, it means that jsdoc comment text is treated as a first-class member of the parse tree now.

Initial scribbles
72f19ef
Compiles but provides spans instead of location pairs
c08258b 
Probably need to fork the services/server types and provide a conversion
with Session.toFileSpan. Not sure where to put the conversion.
Switch to DocumentSpan
2f1d113 
In theory this is already better supported, but not sure practise bears
that out.
Merge branch 'master' into editor-support-for-links
256a60a
Builds w/protocol types + conversions
8ba5dc8
cleanup:better names and scrub TODOs
ca1b273
fix test harness too
867ab34
Misc
5d518d6 
1. Simplify protocol after talking to @mjbvz.
2. Add more tests.
3. Initial notes about where to add parsing.
Parse and store links in the compiler
c07fe40 
The text of the link is still stored in the comment text, but that's now
kept in an object instead of just a string. Each link has the parse for
the entity reference, if there is one.

Needs lots more tests -- this just makes all the existing jsdoc tests
pass.
Merge branch 'master' into editor-support-for-links
82c11fe
more tests and some fixes
90feb3e
Merge branch 'master' into editor-support-for-links
035bb4a
Fix other failing tests
fc813c2
Merge branch 'master' into editor-support-for-links
917314c
fix bad merge
7dd7cab
polish parser
c0e8d47
improve names and array types
8bbc8b4
slight tweaks
070da0b

TypeScript Bot (typescript-bot) commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping Sheetal Nandi (@sheetalkamat), Andrew Casey (@amcasey), Matt Bierner (@mjbvz), Mine Starks (@minestarks) for you. Feel free to loop in other consumers/maintainers if necessary

TypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 8, 2020

Nathan Shively-Sanders (sandersn) commented Dec 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Matt Bierner (@mjbvz) and Andrew Casey (@amcasey), you should maybe also look at this (at least the protocol part)

Andrew Branch (andrewbranch) commented Dec 8, 2020

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this

Only thing I’m curious about is if there are some places where JSDoc can go, but otherwise entity names could not go, and therefore might expose a hole in getSymbolAtLocation.

TypeScript Bot (typescript-bot) commented Dec 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the tarball bundle task on this PR at fce33f1. You can monitor the build here.

Nathan Shively-Sanders (sandersn) commented Dec 8, 2020

Copy link
Copy Markdown
Member Author

Andrew Branch (@andrewbranch) I indeed tried writing more tests and discovered a hole. I'll put up a commit that makes getSymbolAtLocation work with identifiers in links.

Nathan Shively-Sanders (sandersn) commented Mar 5, 2021

Copy link
Copy Markdown
Member Author

The most recent commit avoids breaking VS, and I'm working on tests for all 4 possible combinations of the rich/full matrix.

Nathan Shively-Sanders (sandersn) commented Mar 10, 2021

Copy link
Copy Markdown
Member Author

This is ready for review now.

Jesse Trinity (jessetrinity) reviewed Mar 10, 2021
View reviewed changes
Comment thread src/server/protocol.ts Outdated
Andrew Casey (amcasey) approved these changes Mar 15, 2021
View reviewed changes

Andrew Casey (amcasey) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The behavior and interface changes generally LGTM. Some suggestions below.

Comment thread src/compiler/types.ts
Comment thread src/server/protocol.ts Outdated
Comment thread src/server/protocol.ts
Comment thread src/server/protocol.ts Outdated
Comment thread src/server/protocol.ts Outdated
Comment thread src/services/jsDoc.ts
Comment thread src/services/codefixes/inferFromUsage.ts Outdated
Comment thread tests/cases/fourslash/gotoDefinitionLinkTag1.ts

Nathan Shively-Sanders (sandersn) commented Mar 16, 2021

Copy link
Copy Markdown
Member Author

I think this PR has had enough review to be ready to use. I'll merge it and address any follow-up in subsequent PRs.

Nathan Shively-Sanders (sandersn) deleted the editor-support-for-links branch March 16, 2021 23:26

Wenlu Wang (Kingwl) commented Mar 17, 2021

Copy link
Copy Markdown
Contributor

Cool!

Nathan Shively-Sanders (sandersn) added a commit that referenced this pull request Mar 18, 2021
Revert "Editor support for link tag (#41877)"
271c50c 
This reverts commit ec77bff.
Nathan Shively-Sanders (sandersn) added a commit that referenced this pull request Mar 18, 2021
Revert "Editor support for link tag (#41877)" (#43302)
451d435 
This reverts commit ec77bff.
Nathan Shively-Sanders (sandersn) added a commit that referenced this pull request Mar 19, 2021
Revert "Revert "Editor support for link tag (#41877)" (#43302)"
fb892f0 
This reverts commit 451d435.
Merged
Nathan Shively-Sanders (sandersn) added a commit that referenced this pull request Mar 22, 2021
@link support, second try (#43312)
dcc27eb 
* Revert "Revert "Editor support for link tag (#41877)" (#43302)"

This reverts commit 451d435.

* Fix parsing @link at end of comment

* Parse comments as string when no @link occurs

* fix lint
tmair mentioned this pull request May 10, 2021
Merged
Orta Therox (orta) mentioned this pull request Jun 10, 2021
Open
Brendan Kenny (brendankenny) mentioned this pull request Aug 12, 2021
Merged
Caspar Due (caspardue) mentioned this pull request Aug 29, 2021
Closed

pierre (millsp) commented Sep 30, 2021

Copy link
Copy Markdown
Contributor

Docs can be updated now https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

Andreas Kunz (akudev) mentioned this pull request Dec 29, 2021
Open
Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Daniel Rosenwasser (DanielRosenwasser) DanielRosenwasser left review comments
Andrew Branch (andrewbranch) andrewbranch approved these changes
Sheetal Nandi (sheetalkamat) Awaiting requested review from sheetalkamat
+3 more reviewers
Andrew Casey (amcasey) amcasey approved these changes
Mine Starks (minestarks) minestarks left review comments
Jesse Trinity (jessetrinity) jessetrinity left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

Nathan Shively-Sanders (sandersn)

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

PR Backlog
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Editor support for @see and {@link} in JSDoc comments tags

10 participants

Back | FazBrowse Home | New Git URL