Skip to content

getHovers does not return useful information for an import prefix #32735

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

Closed
DanTup opened this issue Apr 2, 2018 · 7 comments
Closed

getHovers does not return useful information for an import prefix #32735

DanTup opened this issue Apr 2, 2018 · 7 comments
Labels
devexp-server Issues related to some aspect of the analysis server legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@DanTup
Copy link
Collaborator

DanTup commented Apr 2, 2018

Given this code:

import "dart:async" as x;

main() {
  var a = new x.Future();
}

If you send a hover request for the x at the bottom you get this:

{
	"id": "212",
	"result": {
		"hovers": [
			{
				"offset": 148,
				"length": 1,
				"containingLibraryPath": "/Users/dantup/Desktop/Dart Sample/bin/main.dart",
				"containingLibraryName": "",
				"elementDescription": "as x",
				"elementKind": "import prefix",
				"isDeprecated": false
			}
		]
	}
}

There's no information in there to say that this import was for dart:async so it's hard to render a useful tooltip. If I hover over dart:async I get a bunch of information including the dartdoc for the library. It's be nice if we could have this here to show the user information about what x is.

DanTup added a commit to Dart-Code/Dart-Code that referenced this issue Apr 2, 2018
Fixes #761 but we may wish to re-visit once dart-lang/sdk#32735 is fixed/responded to.
@bwilkerson bwilkerson added legacy-area-analyzer Use area-devexp instead. devexp-server Issues related to some aspect of the analysis server type-enhancement A request for a change that isn't a bug P2 A bug or feature request we're likely to work on labels Apr 2, 2018
DanTup added a commit to Dart-Code/Dart-Code that referenced this issue Apr 5, 2018
Fixes #761 but we may wish to re-visit once dart-lang/sdk#32735 is fixed/responded to.
DanTup added a commit to Dart-Code/Dart-Code that referenced this issue Apr 5, 2018
Fixes #761 but we may wish to re-visit once dart-lang/sdk#32735 is fixed/responded to.
DanTup added a commit to Dart-Code/Dart-Code that referenced this issue Apr 5, 2018
Fixes #761 but we may wish to re-visit once dart-lang/sdk#32735 is fixed/responded to.
DanTup added a commit to Dart-Code/Dart-Code that referenced this issue Apr 16, 2018
Fixes #761 but we may wish to re-visit once dart-lang/sdk#32735 is fixed/responded to.
@srawlins
Copy link
Member

Still relevant; no tooltips for prefixes.

@FMorschel
Copy link
Contributor

Working on https://dart-review.googlesource.com/c/sdk/+/390960. I made it work but the current output is not great, still needs some thoughts.

@FMorschel
Copy link
Contributor

FMorschel commented Oct 19, 2024

Something else that crosses my mind today (haven't tried anything just yet) is what should we do when two imports share the same alias?

Something like:

import 'dart:core' as dart;
import 'dart:async' as dart;

When hovering on the acual import line we can differentiate easily enough, but not necessarily when inside the code if both import export a shared top level declaration (say flutter/cupertino and flutter/material both export the base widgets like Text) or when you haven't written the second name yet. Any ideas are welcome.

@DanTup
Copy link
Collaborator Author

DanTup commented Oct 21, 2024

I'm not sure what to do in the case of getHover (which is what the request was initially for, but VS Code no longer users). It might be difficult to do a lot in the case of multiple without protocol changes. Maybe we should either show the first one, or not show anything when there are multiple?

For VS Code / LSP, we're not tied to specifics of the protocol because we're just returning Markdown, so we could probably just duplicate whatever string we should for one when there are multiple.

@bwilkerson
Copy link
Member

We aren't likely to make changes to the legacy protocol to support this. If clients that use the legacy protocol want to support this they could choose to use the LSP request instead.

@FMorschel
Copy link
Contributor

I think the CL is basically working. Does anyone want to review it (or to suggest someone) or should I add you both since you're here on the issue?

@DanTup
Copy link
Collaborator Author

DanTup commented Nov 4, 2024

@FMorschel thanks! I didn't notice in the CL but the text here is wrapped in triple backticks so it has language syntax highlighting applied to it (eg. for is coloured and it's a fixed width font):

image

I can easily fix, but thought I'd ping you in case you wanted to first :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-server Issues related to some aspect of the analysis server legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants