Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Inlay hints for package imports#4502

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
wczyz wants to merge9 commits intohaskell:master
base:master
Choose a base branch
Loading
fromwczyz:package-imports-inlay-hints

Conversation

wczyz
Copy link
Contributor

@wczyzwczyz commentedFeb 19, 2025
edited
Loading

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 wouldclose#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

soulomoon reacted with thumbs up emojisoulomoon and fendor reacted with rocket emoji
@wczyzwczyzforce-pushed thepackage-imports-inlay-hints branch from5ccbfbf tobd0e4caCompareFebruary 19, 2025 12:53
@fendor
Copy link
Collaborator

fendor commentedFeb 20, 2025
edited
Loading

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 theimport (orqualified in the case ofimport qualified)? In theory, we could also display itafter 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 thepackage imports.

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

wczyz reacted with thumbs up emoji

@wczyz
Copy link
ContributorAuthor

wczyz commentedFeb 23, 2025
edited
Loading

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

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

Good point about the case when the user already added somePackageImport, 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 :)

@wczyz
Copy link
ContributorAuthor

Added

  • Displaying afterimport orimport qualified with 1-space padding on the left
  • Handling cases when the user already specified somePackageImport
  • When usingImportQualifiedPost, displaying hint afterimport
  • Tests

I'm still worried that without any configuration options, this change won't be as welcome, but otherwise, it's ready for review.

@wczyzwczyz marked this pull request as ready for reviewMarch 2, 2025 21:33
@wczyzwczyz changed the titleDraft: Inlay hints for package importsInlay hints for package importsMar 2, 2025
@wczyzwczyzforce-pushed thepackage-imports-inlay-hints branch from1b082a9 to3ab746dCompareMarch 3, 2025 01:54
@fendorfendor added the status: needs reviewThis PR is ready for review labelMar 5, 2025
@michaelpj
Copy link
Collaborator

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!

@fendor
Copy link
Collaborator

Sorry for taking so long to come back to you!

In addition to the above (configuration option), I think it would make sense to hide the implementation in a Rule, similarly to howinlayHintProvider does it.
At last, it may also make sense to piggyback off the ruleImportActions to avoid work duplication, asImportActions already traverses the Imports.

The code itself looks good to me, I have no comments right now.
My only question is regarding the use ofGetParsedModule, why did you decide to only use the parsed module?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@michaelpjmichaelpjAwaiting requested review from michaelpj

@fendorfendorAwaiting requested review from fendor

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
status: needs reviewThis PR is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inlay hint for package imports
3 participants
@wczyz@fendor@michaelpj

[8]ページ先頭

©2009-2025 Movatter.jp