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

Wingman/tactics "hole_severity" option is broken #502

Closed
mirko-plowtech opened this issue Nov 16, 2021 · 16 comments · Fixed by #511
Closed

Wingman/tactics "hole_severity" option is broken #502

mirko-plowtech opened this issue Nov 16, 2021 · 16 comments · Fixed by #511
Labels
type: bug A bug or unintended effect

Comments

@mirko-plowtech
Copy link
Contributor

VS code doesn't allow to setup number options due to a "Invalid type. Expected string"

        "haskell.plugin.tactics.config.hole_severity": {
          "enumDescriptions": [
            "error",
            "warning",
            "info",
            "hint",
            "none"
          ],
          "scope": "resource",
          "description": "The severity to use when showing hole diagnostics.",
          "enum": [
            1,
            2,
            3,
            4,
            null
          ],
          "default": null,
          "type": "string"
        },

^ 1,2, 3, 4 are integers, but "validation" requires string.

  • Op 1. It should use integer (btw, instead of null it could use 0)
  • Op 2. It should use "1","2", "3", "4"
@jneira jneira added the type: bug A bug or unintended effect label Nov 16, 2021
@jneira
Copy link
Member

jneira commented Nov 16, 2021

Many thanks for noting it, good catch!

I have to figure what could be the values matching the language server config type (Maybe DiagnosticSeverity):

https://github.com/haskell/haskell-language-server/blob/ce1f353be354711d905f287d1b22f24c06606409/plugins/hls-tactics-plugin/src/Wingman/LanguageServer.hs#L150

Would you like and have time to open a pr to fix it?

@jneira
Copy link
Member

jneira commented Nov 17, 2021

Ping @berberman as you have the expertise about the server and client config.
I think we could replace the enum keys with the lower case version of the haskell enum (so actual enumDescriptions would work afaiu), but not sure how to make Maybe a, Nothing without using null

@mirko-plowtech
Copy link
Contributor Author

Many thanks for noting it, good catch!

I have to figure what could be the values matching the language server config type (Maybe DiagnosticSeverity):

https://github.com/haskell/haskell-language-server/blob/ce1f353be354711d905f287d1b22f24c06606409/plugins/hls-tactics-plugin/src/Wingman/LanguageServer.hs#L150

Would you like and have time to open a pr to fix it?

Sure, I can try. I will probably work on it this weekend =)

@mirko-plowtech
Copy link
Contributor Author

Many thanks for noting it, good catch!
I have to figure what could be the values matching the language server config type (Maybe DiagnosticSeverity):
https://github.com/haskell/haskell-language-server/blob/ce1f353be354711d905f287d1b22f24c06606409/plugins/hls-tactics-plugin/src/Wingman/LanguageServer.hs#L150
Would you like and have time to open a pr to fix it?

Sure, I can try. I will probably work on it this weekend =)

@jneira Sorry, I tried to fix it, but I guess I failed in the attempt.

I realized that with any config I tried to use, Wingman suggestions are always the same...Tbh, at this point, I'm not sure if I understood correctly how this work, I mean, what behavior should this option modify?)

Btw I tried using string with the following options:

[ "DsError", "DsWarning", "DsInfo", "DsHint", null]

or

   [ "error", "warning", "info", "hint", null]

and also number with:

[1, 2, 3, 4, 0]

^ Despite the error doesn't appear anymore, I didn't find any difference, so probably those options won't work..

@jneira
Copy link
Member

jneira commented Nov 24, 2021

I realized that with any config I tried to use, Wingman suggestions are always the same...Tbh, at this point, I'm not sure if I understood correctly how this work, I mean, what behavior should this option modify?)

many thanks for checking it, i am afraid my knowledge about wingman internals is very limited, maybe @berberman or @isovector could help us to understand the config values or how to effectively test they are bein taken in account

@isovector
Copy link
Contributor

hole_severity is the severity wingman uses to tag holes. The difference between the values is whether or not they show up as warnings, errors or hints in the Diagnostics page in vs-code. But, this feature is working for me in vs-code:

2021-11-24-083042_509x258_scrot

@isovector
Copy link
Contributor

Oh, just kidding. I see it now. This is definitely a bug in the property parser, not in Wingman.

@isovector
Copy link
Contributor

Here's the bug: https://github.com/haskell/haskell-language-server/blob/4b7d13977361c515dad8b2e83f50503ea779150d/hls-plugin-api/src/Ide/Plugin/Properties.hs#L424

This should instead be a function of the enum type? Or maybe it should always be integer?

@isovector
Copy link
Contributor

This is a regression caused by me in haskell/haskell-language-server#1608. I'm not sure what the best fix is though. The test case there uses a string as the JSON serialization, so it's not good enough to always be an integer.

@jneira
Copy link
Member

jneira commented Nov 24, 2021

Maybe we should change the values here, making them fit the parser requirements (i think for enums it should accept the enum constructor names all in lower case)
But @mirko-plowtech tried several alternatives included the mentioned one and does not see a evident difference, maybe with you explanation is easier to check if it works

@isovector
Copy link
Contributor

isovector commented Nov 24, 2021

Not sure about the parser --- this instance is defined upstream

https://github.com/haskell/lsp/blob/bf95cd94f3301fe391093912e6156de7cb5c1289/lsp-types/src/Language/LSP/Types/Diagnostic.hs#L20-L40

and the spec seems to agree that this should be an integer:

https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#diagnosticSeverity

@mirko-plowtech
Copy link
Contributor Author

Not sure about the parser --- this instance is defined upstream

https://github.com/haskell/lsp/blob/bf95cd94f3301fe391093912e6156de7cb5c1289/lsp-types/src/Language/LSP/Types/Diagnostic.hs#L20-L40

and the spec seems to agree that this should be an integer:

https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#diagnosticSeverity

Right, it should use integer instead of string. I tested it locally and it works, here is the PR: #511

@jneira jneira linked a pull request Nov 30, 2021 that will close this issue
@jneira
Copy link
Member

jneira commented Nov 30, 2021

closed by #511

@jneira jneira closed this as completed Nov 30, 2021
@isovector
Copy link
Contributor

I don't think that's a fix --- the file is generated automatically, right?

@jneira
Copy link
Member

jneira commented Dec 1, 2021

we can generate a "template" using hls vscode-extension-schema but we have to include it here manually, as it is not always up to date and we have client specific options

hls vscode-extension-schema does not give us the correct vscode schema for this option:

    "haskell.plugin.tactics.config.hole_severity": {
        "type": "string",
        "scope": "resource",
        "enumDescriptions": [
            "error",
            "warning",
            "info",
            "hint",
            "none"
        ],
        "default": null,
        "description": "The severity to use when showing hole diagnostics. These
 are noisy, but some editors don't allow jumping to all severities.",
        "enum": [
            1,
            2,
            3,
            4,
            null
        ]
    },

note the type "string"
but that is a server specific issue imo, will open a new one in hls

@jneira
Copy link
Member

jneira commented Dec 1, 2021

issue in hls: haskell/haskell-language-server#2423

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A bug or unintended effect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants