Skip to content

Remove unused config in hls-class-plugin #3107

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

Merged

Conversation

July541
Copy link
Collaborator

@July541 July541 commented Aug 15, 2022

While looking on #3105, I found type lens itself has a config filed, see haskell-language-server generate-default-config

Remove a duplicate one, although I don't know where is the another one from now.

@michaelpj
Copy link
Collaborator

I'm slightly confused, I think we probably do want a config field for the lenses? Or are there currently two or something?

@July541
Copy link
Collaborator Author

July541 commented Aug 16, 2022

Currently we have two config fields for lens, haskell-language-server generate-default-config shows

"class": {
                "codeActionsOn": true,
                "codeLensOn": true,
                "config": {
                    "typelensOn": true
                }
            },

Turns out codeLensOn and typelensOn are the same one.

@michaelpj
Copy link
Collaborator

Huh, do we generate a generic config option for any plugin that provides code lenses? That's pretty cool.

@July541
Copy link
Collaborator Author

July541 commented Aug 16, 2022

Huh, do we generate a generic config option for any plugin that provides code lenses? That's pretty cool.

Obviously, it does! Although I don't know where the code is due to time limitation.

@michaelpj
Copy link
Collaborator

@berberman ?

@michaelpj
Copy link
Collaborator

Dumb question: does the other config option work? Can we keep the test and use the generic config option? Or maybe we can trust it, I'm just suspicious.

@July541
Copy link
Collaborator Author

July541 commented Aug 16, 2022

Both codeLensOn and typelensOn are available. For other plugins, config fields work fine for me.

Can we keep the test and use the generic config option?

It's better if we can(Just because there exists one already). I tried to find how to write the config, but failed as I said.

Or maybe we can trust it

It's ok I think, all of our plugins don't have tests for main ability, we just write tests for custom config.

@berberman
Copy link
Collaborator

@berberman ?

Yes:

-- This function captures ide methods registered by the plugin, and then converts it to kv pairs
handlersToGenericDefaultConfig :: DSum.DSum IdeMethod f -> [A.Pair]
handlersToGenericDefaultConfig (IdeMethod m DSum.:=> _) = case m of
STextDocumentCodeAction -> ["codeActionsOn" A..= True]
STextDocumentCodeLens -> ["codeLensOn" A..= True]
STextDocumentRename -> ["renameOn" A..= True]
STextDocumentHover -> ["hoverOn" A..= True]
STextDocumentDocumentSymbol -> ["symbolsOn" A..= True]
STextDocumentCompletion -> ["completionOn" A..= True]
STextDocumentPrepareCallHierarchy -> ["callHierarchyOn" A..= True]
_ -> []

So I think we no longer need the typelensOn in this case.

@michaelpj
Copy link
Collaborator

We really need to document how this stuff works...

@michaelpj michaelpj merged commit 92f4bd4 into haskell:master Aug 16, 2022
@July541
Copy link
Collaborator Author

July541 commented Aug 17, 2022

We should have a tech doc to land our tech details while developing hls, but not sure what should be included yet.

sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request Sep 12, 2022
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.

3 participants