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

Display tsserver tags from docstrings in GetDoc and extra_menu_info #1716

Merged
merged 1 commit into from
Dec 30, 2023

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Oct 14, 2023

Fixes #1675


This change is Reviewable

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Out of curiosity, What other tags are there?

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)


ycmd/completers/typescript/typescript_completer.py line 915 at r1 (raw file):

    extra_info = []
    for tag in info.get( 'tags', [] ):

Worth pulling this block into a func? Seems to be duplicated.

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Fixed flake8 errors.

There's a ton of tags:
https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

I'm open for a more general solution if we can think of one.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Taking a look at the tags, this pull request is incomplete. We miss arg as an alternative to param and return as the alternative to returns and probably a lot more.

Maybe we should take all tags and display them as tag_name: tag_text? Maybe without :?

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @puremourning)


ycmd/completers/typescript/typescript_completer.py line 915 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Worth pulling this block into a func? Seems to be duplicated.

Will do, but let's first decide how we want to handle other tags.

@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

Merging #1716 (cba73cb) into master (47baf60) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1716   +/-   ##
=======================================
  Coverage   95.45%   95.46%           
=======================================
  Files          83       83           
  Lines        8141     8155   +14     
  Branches      163      163           
=======================================
+ Hits         7771     7785   +14     
  Misses        320      320           
  Partials       50       50           

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Rebased. Ready for another round of review.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/completers/typescript/typescript_completer.py line 915 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Will do, but let's first decide how we want to handle other tags.

Done.

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Fixed the javascriptreact test.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic)


ycmd/completers/typescript/typescript_completer.py line 1133 at r3 (raw file):

      extra_info.append( 'returns ' + tag_text )
    elif tag_text:
      extra_info.append( tag_text )

worth including the name? what do these look like?

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/completers/typescript/typescript_completer.py line 1133 at r3 (raw file):

Previously, puremourning (Ben Jackson) wrote…

worth including the name? what do these look like?

It's the name of the tag, so in case of @arg a - thing the name is arg and a - thing is the text.

At least for arguments it felt redundant. I honestly don't know about the rest...

Here's the list of all officially defined tags:

https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/completers/typescript/typescript_completer.py line 1133 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

It's the name of the tag, so in case of @arg a - thing the name is arg and a - thing is the text.

At least for arguments it felt redundant. I honestly don't know about the rest...

Here's the list of all officially defined tags:

https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

Included the tag name as well.

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r2, 3 of 3 files at r4, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)


ycmd/tests/typescript/subcommands_test.py line 629 at r4 (raw file):

              'function single_argument_with_doc(a: string): string\n\n'
              'A function with a single argument\n\n'
              'a - The argument\n'

Wait why no ‘param:’ ?

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/tests/typescript/subcommands_test.py line 629 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Wait why no ‘param:’ ?

Fixed.

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

@puremourning puremourning added the Ship It! Manual override to merge a PR by maintainer label Dec 30, 2023
Copy link
Contributor

mergify bot commented Dec 30, 2023

Thanks for sending a PR!

@mergify mergify bot merged commit 9e43034 into ycm-core:master Dec 30, 2023
17 of 18 checks passed
bstaletic added a commit to ycm-core/YouCompleteMe that referenced this pull request Feb 18, 2024
Changes since the last update:

ycm-core/ycmd#1728 YcmShowDetailedDiagnostic should now match what is echoed on the command line.
ycm-core/ycmd#1731 Add support for `workspace/didChangeWorkspaceFolders` LSP notification.
ycm-core/ycmd#1730 LSP servers are now updated with latest server state afer reset
ycm-core/ycmd#1727 Update JDT to version 1.31.0
ycm-core/ycmd#1726 C# symbol search filtering fix
ycm-core/ycmd#1724 C# GoToDocumentOutline consistency patch
ycm-core/ycmd#1723 Implement GoToDocumentOutline in C#
ycm-core/ycmd#1716 Display tsserver tags in detailed info and GetDoc

`workspace/didChangeWorkspaceFolders` seems to be working fine for java,
rust and go, but should be considered experimental. Definitely more
field experience is needed.
What it should do is allow LSP servers to understand multiple projects
in the same vim instance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS: include @param and @returns in docs
2 participants