Skip to content

Support PackageImports in hiddenPackageSuggestion#4537

Merged
fendor merged 2 commits into
haskell:masterfrom
linj-fork:pr/cabal-add-package-imports
Apr 3, 2025
Merged

Support PackageImports in hiddenPackageSuggestion#4537
fendor merged 2 commits into
haskell:masterfrom
linj-fork:pr/cabal-add-package-imports

Conversation

@jian-lin

@jian-lin jian-lin commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator

Fix: #4479

TODO: Probably I need to add more tests.

CC @VeryMilkyJoe @fendor

Video demo

hls-4479-demo.mp4

More info

Here is the string being matched.

With PackageImports

not found: Could not find module ‘Data.List.Split’
Perhaps you meant
  Data.List.Split (needs flag -package-id split-0.2.5)
  Data.List.Split (needs flag -package-id split-0.2.5)

Without PackageImports

not found: Could not load module ‘Data.List.Split’
It is a member of the hidden package ‘split-0.2.5’.
Perhaps you need to add ‘split’ to the build-depends in your .cabal file.
It is a member of the hidden package ‘split-0.2.5’.
Perhaps you need to add ‘split’ to the build-depends in your .cabal file.

I also notice that the "not found" error remains even after the dependency is added by the code action (with and without PackageImports). That may be a bug of my lsp client (eglot on Emacs) or HLS. Needs further investigation.

@jian-lin
jian-lin requested a review from fendor as a code owner April 2, 2025 18:20
@fendor
fendor requested a review from VeryMilkyJoe April 3, 2025 12:14
Comment thread plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/CabalAdd.hs Outdated
Comment thread plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/CabalAdd.hs Outdated
@jian-lin
jian-lin force-pushed the pr/cabal-add-package-imports branch from 48e4cc5 to 39d3f61 Compare April 3, 2025 16:46
@jian-lin
jian-lin requested a review from fendor April 3, 2025 16:48

@jian-lin jian-lin left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have addressed all previous comments. Please take another look.

Comment thread plugins/hls-cabal-plugin/test/CabalAdd.hs
Comment thread plugins/hls-cabal-plugin/test/CabalAdd.hs

@fendor fendor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for your contribution ❤️

@jian-lin

jian-lin commented Apr 3, 2025

Copy link
Copy Markdown
Collaborator Author

Thanks for your review! I had a great experience in my first HLS PR.

@fendor
fendor enabled auto-merge (squash) April 3, 2025 17:12

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also notice that the "not found" error remains even after the dependency is added by the code action (with and without PackageImports). That may be a bug of my lsp client (eglot on Emacs) or HLS. Needs further investigation.

Oh, it is indeed a known HLS bug. With HLS 2.10.0.0 and vscode 2.5.3, I need to manually restart HLS or saving hie.yaml after adding dependencies to .cabal file to let HLS find the newly added dependencies.

In addition, there seems to be a bug in my preferred setting: Emacs with eglot as a LSP client. In that setting, only manual restart works and saving hie.yaml has no effect.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sounds correct!

@fendor
fendor disabled auto-merge April 3, 2025 19:46
@fendor

fendor commented Apr 3, 2025

Copy link
Copy Markdown
Collaborator

@jian-lin It looks like you need to rebase your PR or merge changes from master into it before we can merge :)

@jian-lin
jian-lin force-pushed the pr/cabal-add-package-imports branch from 39d3f61 to 4efa5d7 Compare April 3, 2025 19:50
@jian-lin

jian-lin commented Apr 3, 2025

Copy link
Copy Markdown
Collaborator Author

@fendor Rebased.

@fendor
fendor enabled auto-merge (squash) April 3, 2025 19:53

@VeryMilkyJoe VeryMilkyJoe left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@fendor
fendor merged commit 42a5261 into haskell:master Apr 3, 2025
@jian-lin
jian-lin deleted the pr/cabal-add-package-imports branch April 3, 2025 20:28
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.

Add Package doesn't work with package-imports

3 participants