Use default config on missing configuration section #459

aufarg · 2020-10-03T05:28:47Z

Hi, I've made a change to make default configuration to be used if initializationOptions doesn't have haskell or languageServerHaskell key in it. There are some issue where the server returns haskell-lsp: configuration parse error.. This change shouldn't break any existing configuration because it adds safety net instead of straight failing.

Also, I actually made the same change to haskell/haskell-ide-engine#1802 first before realizing there's a new project. I'm not sure if this can be considered as bug fix, so I'll keep it open there.

hls-plugin-api/src/Ide/Plugin/Config.hs

jneira · 2020-12-23T13:17:09Z

src/Ide/Main.hs

            t <- t
            hPutStrLn stderr $ "Started LSP server in " ++ showDuration t
            sessionLoader <- loadSession dir
-            -- config <- fromMaybe defaultLspConfig <$> getConfig
+            config <- fromMaybe def <$> getConfig


What about dont add the DefaultConfig constructor, keeping the actual definition and only check here that there is no client config doing a pattern matching on Maybe.

Since we make the parsing always successful, I believe getConfig will never return Nothing.

I'm starting to think that to add alert message that we fallback to default config will require a change in haskell-lsp instead around these lines here. There should be a way for us to specify the default config when parsing error occurs.

* Suggest missing imports via package exports map At the expense of some space and initialization time, suggest imports now is able to find suggestions in all the packages available to the project. * BadDependency - include the key in the error message * remove the assumption that the GhcSession is always available * fix bad spacing Co-Authored-By: Moritz Kiefer <moritz.kiefer@purelyfunctional.org> * Add type annotation to clarify rule being defined * Remove file dependency from PackageExports rule * Guess patterns Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>

jneira · 2021-01-11T09:26:56Z

Hi, there is a recent open issue about that (#1196) and the fix is already here so i think we have to unblock this.
I dont fully like the change in the Config data type and not sure if there could be another alternative to signal the client is providing an empty config.

// cc @Hogeyama @aufarg @Ailrun

pepeiborra · 2021-01-11T20:51:28Z

Hi, there is a recent open issue about that (#1196) and the fix is already here so i think we have to unblock this.
I dont fully like the change in the Config data type and not sure if there could be another alternative to signal the client is providing an empty config.

// cc @Hogeyama @aufarg @Ailrun

How do we know that this fix works? I see no test

pepeiborra · 2021-01-11T20:59:43Z

hls-plugin-api/src/Ide/Plugin/Config.hs

+data Config
+  = DefaultConfig
+  | UserConfig Bool Bool Int Int Bool Bool Bool T.Text


This is not good enough. Please restore the field names or use Haddock comments to label the fields

pepeiborra · 2021-01-11T21:07:42Z

The parser should be able to handle the empty object, since that's a valid configuration as all the configuration flags are optional. On the other hand, if a field is the wrong type, the parser should fail and HLS should error out.

The approach taken by this PR is reasonable, but the implementation is a bit convoluted. I would handle this by making all the configuration fields optional, which is equivalent but easier to understand.

aufarg · 2021-01-12T02:48:53Z

Hi, there is a recent open issue about that (#1196) and the fix is already here so i think we have to unblock this.
I dont fully like the change in the Config data type and not sure if there could be another alternative to signal the client is providing an empty config.
// cc @Hogeyama @aufarg @Ailrun

How do we know that this fix works? I see no test

Sorry I haven't write the test because I wasn't sure if it should be done like this. I'll add the tests soon.

aufarg · 2021-01-16T17:41:16Z

The parser should be able to handle the empty object, since that's a valid configuration as all the configuration flags are optional. On the other hand, if a field is the wrong type, the parser should fail and HLS should error out.

The approach taken by this PR is reasonable, but the implementation is a bit convoluted. I would handle this by making all the configuration fields optional, which is equivalent but easier to understand.

I actually have a question for the suggested approach here. I'm quite new to Haskell so I'm not familiar how we do things in Haskell.

So one of the comment I received above asked to make it so if user typed a wrong configuration key, we should print a message. If we make the configuration fields all optional, how do we determine if a value of Config not from user input? Should we check that all the fields are Nothing?

I also have question regarding potential code duplication if we resolve Config until the value is used. Is there maybe a way to resolve the value of Config that will be global from HLS other than in parseConfig?

pepeiborra · 2021-01-17T14:25:05Z

The parser should be able to handle the empty object, since that's a valid configuration as all the configuration flags are optional. On the other hand, if a field is the wrong type, the parser should fail and HLS should error out.
The approach taken by this PR is reasonable, but the implementation is a bit convoluted. I would handle this by making all the configuration fields optional, which is equivalent but easier to understand.

I actually have a question for the suggested approach here. I'm quite new to Haskell so I'm not familiar how we do things in Haskell.

So one of the comment I received above asked to make it so if user typed a wrong configuration key, we should print a message. If we make the configuration fields all optional, how do we determine if a value of Config not from user input? Should we check that all the fields are Nothing?

If a user makes a typo in the configuration key, this will not result in an error with the current implementation. Is that what you are asking?

I also have question regarding potential code duplication if we resolve Config until the value is used. Is there maybe a way to resolve the value of Config that will be global from HLS other than in parseConfig?

What do you mean by resolve here? Perhaps this blog post is helpful: https://reasonablypolymorphic.com/blog/higher-kinded-data/

aufarg · 2021-01-17T15:01:06Z

If a user makes a typo in the configuration key, this will not result in an error with the current implementation. Is that what you are asking?

Yes, and it also does not log any error message. The reason I introduced a new constructor for Config is to differentiate between user config and default config (when we fail to detect user config).

What do you mean by resolve here? Perhaps this blog post is helpful: https://reasonablypolymorphic.com/blog/higher-kinded-data/

Ah this seems to be what I need here. Thanks! I suppose I can also use it to check if the Config is all Nothing, it means we did not detect any configuration from the user.

pepeiborra · 2021-01-17T15:10:27Z

If a user makes a typo in the configuration key, this will not result in an error with the current implementation. Is that what you are asking?

Yes, and it also does not log any error message. The reason I introduced a new constructor for Config is to differentiate between user config and default config (when we fail to detect user config).

I would prefer to keep this PR focused, and just fix the issue with { }. We can consider the behaviour on { haskell.thisIsATypo: true} in a separate PR.

aufarg · 2021-01-17T15:20:38Z

I would prefer to keep this PR focused, and just fix the issue with { }. We can consider the behaviour on { haskell.thisIsATypo: true} in a separate PR.

Actually if it's just fixing the issue with {} I have a previous commit that keeps the record definition here. What do you think?

pepeiborra · 2021-01-17T15:40:26Z

Looks good to me

aufarg · 2021-01-25T17:40:13Z

@pepeiborra @jneira

Sorry for the delay, took me quite a while to understand lenses for implementing the test. I keep the record definition but it doesn't print any message if the key is not detected for now.

Ailrun

LGTM :)

aufarg · 2021-01-26T15:03:20Z

Hmm, not sure why my test always timed out on (8.10.3, ubuntu-latest)

aufarg · 2021-01-31T08:03:28Z

It seems the server does not always send log notification after config change. I updated the test code to throw dummy response so it'll not block waiting for response.

aufarg · 2021-02-03T16:28:42Z

@Ailrun Were you about to merge this to master? I see that you make a merge commit to merge master to this branch instead.

Ailrun · 2021-02-03T16:32:53Z

I'm actually waiting for @jneira's approval. I made the merge commit to check that it still works with many other recent changes.

jneira

lgtm too, thanks for the pr and the patience!

Ailrun · 2021-02-03T18:59:12Z

It looks like some formatter tests fail. Could you check it?

On serving initialize request, deserializing HIE configuration embedded in InitializeParam passed by client will result in an error if during process the server cannot find HIE specific configuration key under initializationOptions. This commit changes the initializationOptions deserialization to return the default configuration if configuration key cannot be found under initializationOptions. Here, setting the key with a value of null will also be considered as part of not found condition to accommodate clients that fills missing user options as null.

Ailrun reviewed Oct 3, 2020

View reviewed changes

hls-plugin-api/src/Ide/Plugin/Config.hs Show resolved Hide resolved

aufarg force-pushed the return-default-on-empty-init-option branch 2 times, most recently from 993f4a8 to c3c0499 Compare October 4, 2020 05:34

jneira requested changes Dec 23, 2020

View reviewed changes

hls-plugin-api/src/Ide/Plugin/Config.hs Show resolved Hide resolved

jneira reviewed Dec 23, 2020

View reviewed changes

pepeiborra force-pushed the master branch 2 times, most recently from 66ada8c to d4f5d43 Compare December 29, 2020 13:34

jneira mentioned this pull request Jan 11, 2021

Initialization fails if initializationOptions is empty #1196

Closed

pepeiborra reviewed Jan 11, 2021

View reviewed changes

aufarg force-pushed the return-default-on-empty-init-option branch from c3c0499 to f875fe6 Compare January 25, 2021 17:27

Ailrun approved these changes Jan 25, 2021

View reviewed changes

aufarg force-pushed the return-default-on-empty-init-option branch 2 times, most recently from e39c054 to 20de9be Compare January 26, 2021 13:27

aufarg force-pushed the return-default-on-empty-init-option branch from 20de9be to 9b21da1 Compare January 31, 2021 06:00

aufarg requested review from jneira and pepeiborra January 31, 2021 08:04

jneira approved these changes Feb 3, 2021

View reviewed changes

Ailrun added the merge me Label to trigger pull request merge label Feb 3, 2021

aufarg force-pushed the return-default-on-empty-init-option branch from d1b5b71 to 4fa8c8f Compare February 4, 2021 14:30

mergify bot merged commit 0fd73f8 into haskell:master Feb 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use default config on missing configuration section #459

Use default config on missing configuration section #459

aufarg commented Oct 3, 2020

jneira Dec 23, 2020 •

edited

Loading

aufarg Dec 24, 2020

aufarg Dec 24, 2020

jneira commented Jan 11, 2021 •

edited

Loading

pepeiborra commented Jan 11, 2021 •

edited

Loading

pepeiborra Jan 11, 2021 •

edited

Loading

pepeiborra commented Jan 11, 2021

aufarg commented Jan 12, 2021

aufarg commented Jan 16, 2021

pepeiborra commented Jan 17, 2021

aufarg commented Jan 17, 2021

pepeiborra commented Jan 17, 2021

aufarg commented Jan 17, 2021

pepeiborra commented Jan 17, 2021

aufarg commented Jan 25, 2021

Ailrun left a comment

aufarg commented Jan 26, 2021

aufarg commented Jan 31, 2021

aufarg commented Feb 3, 2021

Ailrun commented Feb 3, 2021

jneira left a comment

Ailrun commented Feb 3, 2021

Use default config on missing configuration section #459

Use default config on missing configuration section #459

Conversation

aufarg commented Oct 3, 2020

jneira Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

aufarg Dec 24, 2020

Choose a reason for hiding this comment

aufarg Dec 24, 2020

Choose a reason for hiding this comment

jneira commented Jan 11, 2021 • edited Loading

pepeiborra commented Jan 11, 2021 • edited Loading

pepeiborra Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

pepeiborra commented Jan 11, 2021

aufarg commented Jan 12, 2021

aufarg commented Jan 16, 2021

pepeiborra commented Jan 17, 2021

aufarg commented Jan 17, 2021

pepeiborra commented Jan 17, 2021

aufarg commented Jan 17, 2021

pepeiborra commented Jan 17, 2021

aufarg commented Jan 25, 2021

Ailrun left a comment

Choose a reason for hiding this comment

aufarg commented Jan 26, 2021

aufarg commented Jan 31, 2021

aufarg commented Feb 3, 2021

Ailrun commented Feb 3, 2021

jneira left a comment

Choose a reason for hiding this comment

Ailrun commented Feb 3, 2021

jneira Dec 23, 2020 •

edited

Loading

jneira commented Jan 11, 2021 •

edited

Loading

pepeiborra commented Jan 11, 2021 •

edited

Loading

pepeiborra Jan 11, 2021 •

edited

Loading