Skip to content

hls-explicit-fixity-plugin #2941

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Aug 11, 2022
Merged

hls-explicit-fixity-plugin #2941

merged 14 commits into from
Aug 11, 2022

Conversation

July541
Copy link
Collaborator

@July541 July541 commented Jun 5, 2022 •
edited
Loading

Close #2019.

This plugin will show available fixity explicitly while hovering.

The basic code is done, tests will come soon... Done

fixity1
fixity2

@July541 July541 requested review from jneira and pepeiborra as code owners June 5, 2022 15:33
@July541 July541 force-pushed the hls-explicit-fixity-plugin branch from 5eeb0c5 to ea8dc5d Compare June 5, 2022 15:36
@July541
Copy link
Collaborator Author

July541 commented Jun 5, 2022

The license missing... will upload it tomorrow...

pepeiborra
pepeiborra reviewed Jun 5, 2022
View reviewed changes
plugins/hls-explicit-fixity-plugin/src/Ide/Plugin/ExplicitFixity.hs Outdated
Comment on lines 43 to 48
-- Get fixity from HscEnv for local defined operator will crash the plugin,
-- we first try to use ModIface to get fixities, and then use
-- HscEnv if ModIface doesn't available.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you look into getting the fixity from the HieAst ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried HieAst but found nothing, it would be appreciated if you can leave some hints :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @wz1000

pepeiborra
pepeiborra reviewed Jun 5, 2022
View reviewed changes
Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

I think you need to add the testsuite to CI

@July541 July541 force-pushed the hls-explicit-fixity-plugin branch from ea8dc5d to e5fe94a Compare June 6, 2022 06:48
@michaelpj
Copy link
Collaborator

I think we should consider rolling this into the main hover code. While theoretically we can have multiple hover plugins, the hovers get concatenated together which isn't the best way of doing things. I think fixity is uncontroversially useful enough that we could just make it part of the default hover. It might save a bunch of code, too.

@July541
Copy link
Collaborator Author

July541 commented Jun 7, 2022

I think we should consider rolling this into the main hover code. While theoretically we can have multiple hover plugins, the hovers get concatenated together which isn't the best way of doing things. I think fixity is uncontroversially useful enough that we could just make it part of the default hover. It might save a bunch of code, too.

It turns out I'm a little fear of the performance requirements of ghcide. And it not easy to make 'show fixity' configurable, I can move this to ghcide if we reach a consensus on this plugin's performance and decide it must be in hover.

@July541
Copy link
Collaborator Author

July541 commented Jun 7, 2022

One more step, I even want hls-hover-plugin to supply hover functionality.

@michaelpj
Copy link
Collaborator

And it not easy to make 'show fixity' configurable, I can move this to ghcide if we reach a consensus on this plugin's performance and decide it must be in hover.

Configuration is bad :) The best option is for it to be fast enough and on by default!

@July541
Copy link
Collaborator Author

July541 commented Jun 7, 2022

I'll move this to ghcide later if nobody suspects the performance of the current implementation.

@pepeiborra
Copy link
Collaborator

pepeiborra commented Jun 8, 2022 •
edited
Loading

I am not worried about the performance, but I would prefer if we fixed how hover plugins get combined rather than putting all the hovers in ghcide.
We can learn some tricks of how optparse-applicative renders help text for instance

@michaelpj
Copy link
Collaborator

I am not worried about the performance, but I would prefer if we fixed how hover plugins get combined rather than putting all the hovers in ghcide.

I think it's not too bad, I just think that in this instance, since it's such a core piece of information, we might as well put it in the main hover. If e.g. someone actually writes code to show strictness signatures in a hover, that should definitely go in a plugin!

pepeiborra
pepeiborra approved these changes Jun 19, 2022
View reviewed changes
Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Maybe we should land this now and move it to ghcide once we are 100% happy with it

@July541
Copy link
Collaborator Author

July541 commented Jun 19, 2022

I've been busy with graduation these days, and I'll pick up my opened prs later :)

@July541 July541 mentioned this pull request Jun 20, 2022
Closed
@pepeiborra
Copy link
Collaborator

did we end up putting this hover in ghcide?

@July541
Copy link
Collaborator Author

July541 commented Jul 10, 2022

did we end up putting this hover in ghcide?

I initially want close this after #2973 is merged.

@July541 July541 requested a review from pepeiborra August 9, 2022 09:11
@July541
Copy link
Collaborator Author

July541 commented Aug 9, 2022

Continue in this thread because #2973 (comment)

@July541 July541 force-pushed the hls-explicit-fixity-plugin branch from 4cddad0 to f0bb1c1 Compare August 9, 2022 10:22
Ailrun
Ailrun approved these changes Aug 11, 2022
View reviewed changes
Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

Thank you for the implementation! Looks good to me overall.

Here are my two cents.

@Ailrun Ailrun merged commit 0e74593 into haskell:master Aug 11, 2022
@July541 July541 mentioned this pull request Aug 12, 2022
Closed
sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request Sep 12, 2022
@July541 @pepeiborra @sloorush
hls-explicit-fixity-plugin (haskell#2941)
c9516e7 
* init hls-explicit-fixity-plugin

* Update dependencies

* Refactor: Prevent block on startup

* Run pre-commit

* Compatibility: Add emptyMessages

* Remove unused ModIface

* Comment why to make this plugin a lower priority

* Provide hover content while testing fail

* Avoid lambda

* 4 space indent

* Pass Text instead of Name

Co-authored-by: Pepe Iborra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelpj michaelpj michaelpj left review comments

@drsooch drsooch drsooch left review comments

@Ailrun Ailrun Ailrun approved these changes

@jneira jneira Awaiting requested review from jneira

@Anton-Latukha Anton-Latukha Awaiting requested review from Anton-Latukha

@pepeiborra pepeiborra Awaiting requested review from pepeiborra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: Fixity information in hover
5 participants
@July541 @michaelpj @pepeiborra @Ailrun @drsooch