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

Draft: Inlay hints for package imports #4502

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wczyz
Copy link

@wczyz wczyz commented Feb 19, 2025

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:

  • Decide where to put it (add to existing plugin / create a new one)
  • Add configurability, disabled by default
  • Tests

This 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:
Screenshot from 2025-02-19 03-33-02

@wczyz wczyz force-pushed the package-imports-inlay-hints branch from 5ccbfbf to bd0e4ca Compare February 19, 2025 12:53
@fendor
Copy link
Collaborator

fendor commented Feb 20, 2025

Hi, thank you for the pull request! This looks quite cool!

The location for the feature looks good to me.
The layout is indeed a bit unfortunate, perhaps it would be better if the inlay hint was immediately after the import (or qualified in the case of import qualified)? In theory, we could also display it after the module name, but that doesn't feel as nicely.

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 package imports.

I haven't read the code, yet, but is the inlay hint displayed also when the user already added some PackageImport?
I imagine this feature works best, if the user doesn't align imports and maybe used ImportQualifiedPost?

@wczyz
Copy link
Author

wczyz commented Feb 23, 2025

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 import / import qualified
As you mentioned, ideally the user doesn't align imports and doesn't qualify them or pushes them to the end with ImportQualifiedPost

Screenshot from 2025-02-23 22-04-00

Good point about the case when the user already added some PackageImport, I'll handle it.
Once we agree on how it should be displayed I'll also add some tests

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inlay hint for package imports
2 participants