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 '(approximate)' to the beginning of quick info requests in PartialSemantic mode #40061

Merged
merged 2 commits into from
Aug 15, 2020

Conversation

DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Aug 14, 2020

image

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Aug 14, 2020
@DanielRosenwasser DanielRosenwasser changed the title Add '(approximate)' to the beginning of quick info requests. Add '(approximate)' to the beginning of quick info requests in PartialSemantic mode Aug 14, 2020
@DanielRosenwasser
Copy link
Member Author

@typescript-bot cherry-pick this to release-4.0

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 14, 2020

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.0 on this PR at 54f988c. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've opened #40062 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Aug 14, 2020
Component commits:
54f988c Add '(approximate)' to the beginning of quick info requests.
Copy link
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

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

Not sure I like the line break. I'd just have it prefix the quick info on the same line. Also, maybe a better prefix is "(approximation)". Otherwise, looks good.

@DanielRosenwasser
Copy link
Member Author

Well, I did try both. Here's what same line looked like.

image

@ahejlsberg
Copy link
Member

I like the single line better!

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Aug 15, 2020

It's sort of a shame that "partial mode" is really something up to interpretation by each editor. Otherwise, I'd just say we should display "loading" which works kind of nicely when the semantic server comes back with a response.

loadingpartial

@DanielRosenwasser
Copy link
Member Author

This is what it'll look like

image

@DanielRosenwasser DanielRosenwasser merged commit 2426eb4 into master Aug 15, 2020
@DanielRosenwasser
Copy link
Member Author

@typescript-bot cherry-pick this to release-4.0 and LKG

@typescript-bot
Copy link
Collaborator

Heya @DanielRosenwasser, I'm starting to run the task to cherry-pick this into release-4.0 on this PR at 05f1301. Hold tight - I'll update this comment with the log link once the build has been queued.

DanielRosenwasser added a commit that referenced this pull request Aug 15, 2020
…lSemantic mode (#40061)

* Add '(approximate)' to the beginning of quick info requests.

* Use 'approximation' instead of 'approximate'.
@DanielRosenwasser DanielRosenwasser deleted the approximateQuickInfo branch August 15, 2020 01:03
@sheetalkamat
Copy link
Member

Should we do this for completion details too?

@mjbvz
Copy link
Contributor

mjbvz commented Aug 15, 2020

Sorry @DanielRosenwasser, just seeing this now. I like the single line version.

A few questions:

  • I think we may need to add some use facing explanation around approximation. Such as in the hover contents, have a string that says something like, this result is an best effort approximation because the project is still loading (with better phrasing)

  • I believe VS Code could have determined this itself? Or are there cases where we can be sure the the partial server is returning the full result? If TS Server does need to tell us which results are approximate, would a new flag on the response object be better so that editors can determine how to display it?

@DanielRosenwasser
Copy link
Member Author

I think ideally the editor would display this itself (and is capable of doing so); if we think we can get something to do this in the August release of VS Code, then I would say we should back out the change now.

@DanielRosenwasser DanielRosenwasser restored the approximateQuickInfo branch August 16, 2020 04:26
DanielRosenwasser added a commit that referenced this pull request Aug 16, 2020
typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Aug 16, 2020
Component commits:
1582b76 Revert "Add '(approximate)' to the beginning of quick info requests in PartialSemantic mode (microsoft#40061)"
This reverts commit 2426eb4.
DanielRosenwasser added a commit that referenced this pull request Aug 17, 2020
Component commits:
1582b76 Revert "Add '(approximate)' to the beginning of quick info requests in PartialSemantic mode (#40061)"
This reverts commit 2426eb4.

Co-authored-by: Daniel Rosenwasser <[email protected]>
DanielRosenwasser added a commit that referenced this pull request Aug 17, 2020
@jakebailey jakebailey deleted the approximateQuickInfo branch November 7, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants