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.

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

hls-class-plugin enhancement #2920

Merged
merged 15 commits into from
Jun 22, 2022
Merged

hls-class-plugin enhancement #2920

merged 15 commits into from
Jun 22, 2022

Conversation

July541
Copy link
Collaborator

@July541 July541 commented May 23, 2022 •
edited by gitpod-io bot
Loading

This pr includes:

  1. Add code lens to show missing type signatures for instance methods.
  2. Add with signature code actions.
  3. Add InstanceSigs pragma if needed.

Close #1157

codelens
codeactions

@July541 July541 requested a review from Ailrun as a code owner May 23, 2022 13:21
@July541 July541 requested a review from pepeiborra as a code owner May 23, 2022 14:55
@July541 July541 force-pushed the type-signature branch 2 times, most recently from 8e8b03f to 9283e5f Compare May 23, 2022 15:19
@July541 July541 removed the request for review from pepeiborra May 24, 2022 11:43
Ailrun
Ailrun reviewed May 28, 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.

I will review in more detail once I come back from my vac. However, just one high level Q.

plugins/hls-class-plugin/src/Ide/Plugin/Class/Types.hs
}

rules :: Recorder (WithPriority Log) -> Rules ()
rules recorder = do
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why do you think that it would be better to introduce a new rule?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because rules have cache? I'm not sure about that.

michaelpj
michaelpj reviewed Jun 7, 2022
View reviewed changes
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

I didn't get the whole way through, but here's a partial review. It's a bit hard to tell which bits are old and which are new since you moved things, so I just looked at everything.

plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs Show resolved Hide resolved
plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs
pure Null
where
toTextDocunemtEdit edit =
TextDocumentEdit (VersionedTextDocumentIdentifier uri (Just 0)) (List [InL edit])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit suspicious. I'm not sure how file versions should be handled but probably not like this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is from https://github.com/haskell/haskell-language-server/blob/master/hls-plugin-api/src/Ide/PluginUtils.hs#L135-145

plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs Show resolved Hide resolved
plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs Show resolved Hide resolved
plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs Outdated
$ liftIO
$ runAction "classplugin.codeAction.GetInstanceBindTypeSigs" state
$ use GetInstanceBindTypeSigs docPath
pure $ concatMap mkAction $ minDefToMethodGroups range sigs . classMinimalDef $ cls
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be tempted to move the mkAction part to the caller. That avoids a nested where clause (3 deep now!), and maybe lets you simplify the call site since you're doing various combinations of concatMap/mapM/join.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keep mkAction here due to parameter dependency.

plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeLens.hs Outdated Show resolved Hide resolved
plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeLens.hs Outdated Show resolved Hide resolved
plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeLens.hs Show resolved Hide resolved
plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeLens.hs Show resolved Hide resolved
plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeLens.hs Outdated Show resolved Hide resolved
Ailrun
Ailrun reviewed Jun 8, 2022
View reviewed changes
plugins/hls-class-plugin/README.md Outdated Show resolved Hide resolved
plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs Show resolved Hide resolved
plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs Outdated
= [ mkCodeAction title
$ mkLspCommand plId codeActionCommandId title
(Just $ mkCmdParams methodGroup False)
, mkCodeAction titleWithSig
Copy link
Member

Choose a reason for hiding this comment

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

As I don't use type signatures on class methods very often, I prefer that two-step approach. It is indeed, even though we are not so extreme compared to some other LS, too unwieldy to see a list of code actions.

plugins/hls-class-plugin/test/testdata/T9.hs Outdated Show resolved Hide resolved
plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs Show resolved Hide resolved
@July541
Copy link
Collaborator Author

July541 commented Jun 22, 2022

Sorry for the long delay, I think I've resolved all reviews.

michaelpj
michaelpj approved these changes Jun 22, 2022
View reviewed changes
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Thanks for the persistent work :)

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Jun 22, 2022
@mergify mergify bot merged commit 3461823 into haskell:master Jun 22, 2022
@July541 July541 mentioned this pull request Jun 27, 2022
Merged
@Ailrun Ailrun mentioned this pull request Jun 28, 2022
Closed
This was referenced Jun 28, 2022
Open
Closed
@July541 July541 mentioned this pull request Aug 15, 2022
Closed
hololeap pushed a commit to hololeap/haskell-language-server that referenced this pull request Aug 26, 2022
@July541 @mergify @hololeap
hls-class-plugin enhancement (haskell#2920)
2d2a09a 
* hls-class-plugin enhancement

* Comment to be compatible

* Add HasSrcSpan instances

* hls-class-plugin enhancement

* Comment to be compatible

* Add HasSrcSpan instances

* Compitable fix

* Qualified name

* Fix compatibility

* Resolve reviews

* Rename test files

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ailrun Ailrun Ailrun left review comments

@michaelpj michaelpj michaelpj approved these changes

Assignees
No one assigned
Labels
merge me Label to trigger pull request merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature request] Type sigs on class plugin
3 participants
@July541 @michaelpj @Ailrun