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

Abbreviate explicit import code lenses #2769

Merged
merged 4 commits into from
Mar 15, 2022

Conversation

michaelpj
Copy link
Collaborator

@michaelpj michaelpj commented Mar 9, 2022

The tests currently don't check anything about the titles, I'm unsure
whether it's worth writing a test just for this.

Fixes #2765.

Demo:
image

The tests currently don't check anything about the titles, I'm unsure
whether it's worth writing a test just for this.

Fixes haskell#2765.
@michaelpj michaelpj requested a review from pepeiborra as a code owner March 9, 2022 15:36
@michaelpj michaelpj force-pushed the mpj/abbreviate-code-lenses branch from 0819814 to 6f70bf6 Compare March 9, 2022 15:36
@pepeiborra
Copy link
Collaborator

pepeiborra commented Mar 9, 2022

You could extract the new logic into a pure function. Then you can test it easily without involving lsp-test.

Although ultimately we also want an lsp test to ensure that the logic doesn't get removed accidentally.

@michaelpj
Copy link
Collaborator Author

Great point, I'll add a test for the abbreviation function.

@michaelpj
Copy link
Collaborator Author

I added some tests, which was good, because it was broken :) one day I will not be surprised by this.

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Mar 15, 2022
@mergify mergify bot merged commit 621c2bb into haskell:master Mar 15, 2022
July541 pushed a commit to July541/haskell-language-server that referenced this pull request Mar 30, 2022
* Abbreviate explicit import code lenses

The tests currently don't check anything about the titles, I'm unsure
whether it's worth writing a test just for this.

Fixes haskell#2765.

* Add tests for abbreviation and fix bugs

* Fix a warning
@dariooddenino
Copy link

Hi, this is my version of hls:

❯ haskell-language-server-wrapper --version
haskell-language-server version: 1.6.1.1 (GHC: 9.0.2) (PATH: ~/.local/bin/haskell-language-server-wrapper) (GIT hash: 8466bc11a655f4cfbc8f228e527c2a7bf6762c7f)

which should already include this pr.

I'm still seeing incredibly long code lenses for explicit import. Am I missing anything?

Thanks!

@michaelpj
Copy link
Collaborator Author

I don't think this PR is in a released version yet.

@dariooddenino
Copy link

It looks like this was merged into master 21 days ago, and I manually built hls from a much more recent commit.
Now I feel like I'm missing something incredibly obvious. Sorry for the confusion.

@michaelpj
Copy link
Collaborator Author

Can you provide a reproduction?

@dariooddenino
Copy link

Sure!
https://github.com/dariooddenino/pru

This is what I see from src/Handler/Home.hs:

Screenshot from 2022-04-05 10-14-22

@jhrcek
Copy link
Collaborator

jhrcek commented Apr 5, 2022

The thing I'd check is whether the haskell-language-server-wrapper that you have in your PATH is the actual binary that is being run by your editor. Can you check some tree view of running processes and check the command that your editor used to start hls? E.g on my linux I can use htop --tree to see that my vscode started hls using the following command.
Screenshot from 2022-04-05 10-29-38
You will likely find out that your editor has started different hls binary than the one you're showing version of..

@dariooddenino
Copy link

@jhrcek You are right.
I even checked emacs' path and tried running manually hls from within it, but didn't think about the obvious solution of checking the running process.
Sorry, and thanks to you all!

@michaelpj
Copy link
Collaborator Author

Cannot reproduce indeed:
image

However the "Refine import" lens is still massive and could do with this treatment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abbreviate explicit import code lenses
4 participants