-
-
Notifications
You must be signed in to change notification settings - Fork 378
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
Inlay hints for package imports #4502
base: master
Are you sure you want to change the base?
Conversation
5ccbfbf
to
bd0e4ca
Compare
Hi, thank you for the pull request! This looks quite cool! The location for the feature looks good to me. Regarding configurability, usually we try to have as little configuration as possible as it requires twice the amount of tests, so I think we should just decide on a way to display the I haven't read the code, yet, but is the inlay hint displayed also when the user already added some |
Thanks @fendor I'm fine with not configuring it, but while I would leave it enabled by default, I can imagine many people wouldn't like it. Played around a bit with positioning and IMO it looks best after Good point about the case when the user already added some In an ideal world, it would be nice if inlay hints could replace appropriate amount of spaces, but I don't think that's possible :) |
Added
I'm still worried that without any configuration options, this change won't be as welcome, but otherwise, it's ready for review. |
1b082a9
to
3ab746d
Compare
This seems great! I think the big challenge is just "where to put it?". I think the main constraint is that Inlay hints are supposed to look the same as the text that they would insert when triggered. So I don't think we can put it at the end of the line. Given that we're showing them in the right place syntactically, I don't see how we can avoid messing up people's formatting. We generally don't try to match this (see also the inlay hints for imports, which can't easily match people's preferred formatting either). So overall I don't see much alternative to what you're doing. I do think this is likely to be a bit of a "love-it-or-hate-it" feature, so we probably will need the ability to turn it off! |
Hey!
I sometimes find myself trying to figure out which package certain imports come from, so when I saw #4485, I thought a good first contribution would be to implement inlay hints for package imports.
This PR isn’t finished yet, but I’d appreciate any feedback on whether I’m headed in the right direction.
I’m uncertain about where exactly this functionality should live. Do you think there's a plugin to which it could be added or would a new plugin be more appropriate? For now, I put it in a somewhat related plugin as a proof of concept.
TODO:
Add configurability, disabled by defaultThis change would close #4485.
After implementing I have mixed feelings about it because it messes with the layout when imports are neatly aligned with whitespace (as it is in hls repo).

Here's what it looks like: