Skip to content

Hovers for import prefixes should show valid Dart code for the import #57020

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 Nov 4, 2024 · 3 comments
Closed

Hovers for import prefixes should show valid Dart code for the import #57020

DanTup opened this issue Nov 4, 2024 · 3 comments
Labels
devexp-server Issues related to some aspect of the analysis server legacy-area-analyzer Use area-devexp instead. P3 A lower priority bug or feature request

Comments

@DanTup
Copy link
Collaborator

DanTup commented Nov 4, 2024

#32735 improved the hovers for import prefixes, but the new text is in a field that usually contains valid Dart code and LSP wraps in triple backticks. This results in syntax highlighting being applied to English text:

image

To maintain consistency, rather than removing the backticks we should update the element descriptions for import prefixes to be the valid Dart code that adds the relevant imports instead.

(cc @FMorschel)

@DanTup DanTup added legacy-area-analyzer Use area-devexp instead. devexp-server Issues related to some aspect of the analysis server labels Nov 4, 2024
@scheglov scheglov added the P3 A lower priority bug or feature request label Nov 4, 2024
@bwilkerson
Copy link
Member

The question then becomes, which imports to show. I'll repeat the comment I made on Discord (which assumes that we'll only display imports that have the prefix being hovered over:

For the case of multiple imports we have three options that I can think of. The easiest is to just list all of the imports that use that prefix and let the user guess which ones are relevant. (You can probably tell by the way I phrased it that it isn't my favorite option. 🙂 ) The second is to list the first import that includes the element in its namespace. The third is to include all of the imports that include the element in their namespace. Given that the namespaces are already computed at the point where we're computing the hover text, I suspect that any of those options would be performant enough.

When I said "in it's namespace" it might not have been clear what I meant. I meant that we could consider including only those imports from which the name being referenced through the prefix can be imported.

I might misunderstand the purpose of the hover, but I assumed it was to help identify which import was being referenced in that case. If not, then my comments aren't relevant.

@FMorschel
Copy link
Contributor

As I wrote there as well:

For consistency I believe we should always show all imports with the prefix in the order they're in.

Although I like the idea of sorting the elements or limiting them to the current namespace, it would not be consistent in the entirety of the library and that is something I would prefer, so that users are always aware of what would appear and in what order.

I see your point in limiting it, I just don't think it would be as consistent because you can either hover the prefix on a declaration like a class or a function, but you could also hover it by itself or at the import line.

I'd rather show all prefixed imports everywhere so that people are aware of what the actual prefix fully means.

I would not expect real code to have multiple imports using the same prefix to be a really common thing but when they appear, I'd like to show the user that this is being done so that they are aware every time this might be causing some confusion.

I might misunderstand the purpose of the hover, but I assumed it was to help identify which import was being referenced in that case.

Although I see some truth in this, it might not be the only reason why this hover would be relevant. I think for what you are talking about here, three other issues would fit this category better (I think in this specific order of relevance but the first and second don't have that much priority over one another):

@FMorschel
Copy link
Contributor

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. P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

4 participants