Store the lsp client settings in shakeExtras and create a Rule to get them#731
Store the lsp client settings in shakeExtras and create a Rule to get them#731pepeiborra merged 18 commits intohaskell:masterfrom jneira:client-settings-da
Conversation
jneira
changed the title
pepeiborra
left a comment
pepeiborra
left a comment
There was a problem hiding this comment.
Looks sensible to me! Please take a look at my comments about early cutoff. Moreover, I agree that it is a good idea to restart the Shake session (with the usual setSomethingModified) whenever the config changes, please add that too.
Could we have some tests to at least check that the Shake session is restarted when the config does change?
|
@pepeiborra many thanks for your comments and insights, i was a little bit lost in the ghcide codebase. |
jneira
changed the title
I'll try to add it asap. Only comment that i've cherrypicked changes here in hls and now hlint suggestions honour happily the client setting 😄 |
|
needs rebase (and tests!) |
|
rebased! that was easy, implement the tests will take me some time thought cause i am not used to ghcode test suite, any tip will be welcomed 😄 |
|
The test should do something along these lines:
|
|
Yeah, i was thinking in add a test in hls with hlint, cause there it is easy to observe how hlints diagnostics come and go. |
|
I think we want to have a test here as well please. You can send custom messages to the lsp client if necessary, see how cradle loading is tested |
|
/azp run |
|
No pipelines are associated with this pull request. |
|
I cant make progress with the tests cause the test suite hangs in windows. hls one too so i suspect some change in lsp-test around the shutdown of the process is involved. 😟 |
|
After fixing the hang of the test suite in windows and add some logging notifications in the Shake module i have been able to add the test cases. |
|
/azp run |
|
Pull request contains merge conflicts. |
|
rebase needed |
|
There is a failing test for ghc-8.6 and 8.10 (but no for 8.8 🤔 ) I guess the new log notifications for shake session broke it. 😟 |
pepeiborra
left a comment
pepeiborra
left a comment
There was a problem hiding this comment.
There is a failing test for ghc-8.6 and 8.10 (but no for 8.8 🤔 )
2020-09-20T21:57:49.0532485Z Interface loading tests 2020-09-20T21:57:49.2935216Z iface-error-test-1: FAIL 2020-09-20T21:57:49.2959459Z Exception: NamedParserException "NotificationMessage ServerMethod PublishDiagnosticsParams" (NamedParserException "NotificationMessage ServerMethod PublishDiagnosticsParams" (NamedParserException "NotificationMessage ServerMethod PublishDiagnosticsParams" (NamedParserException" .............. (NamedParserException "Response for id: IdInt 1" (NamedParserException "NotificationMessage ServerMethod (ProgressParams WorkDoneProgressBeginParams)" (BothFailed (Unexpected "ConduitParser.empty") (Unexpected "ConduitParser.empty"))))))))))))))))))))))))))))) (Unexpected "ConduitParser.empty")))))))))))))))))))))))))))))I guess the new log notifications for shake session broke it. 😟
You can probably simplify the failing test as below, which might help it pass:
res <- skipManyTill anyMessage $ responseForId li
|
Really tired of fpcomplete timeouts 😢 |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
🤞 |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
finally ci is green! @pepeiborra i think last requested changes are done, thanks for your patience 😄 |
|
Eureka! |
… them (haskell#731) * Store client settings in ide state * Log ide config registered in initHandler * Use a Maybe aware updater function * Create a Rule to get client settings * Create a specific getter for client settings * Trim trailing whitespace * Use modifyVar to avoid race conditions * Add comment to GetClientSettings * Use defineEarlyCutOffNoFile for GetClientSettings * Restart shake on config changed * Use Hashed for clientSettings * Send log notifications to client about session * Show test output directly * Add tests over client settings * Apply hlint hints * Simplify iface test to make it more robust Following @pepeiborra advise * Send session notifications only in test mode * Retry bench execution
… them (haskell/ghcide#731) * Store client settings in ide state * Log ide config registered in initHandler * Use a Maybe aware updater function * Create a Rule to get client settings * Create a specific getter for client settings * Trim trailing whitespace * Use modifyVar to avoid race conditions * Add comment to GetClientSettings * Use defineEarlyCutOffNoFile for GetClientSettings * Restart shake on config changed * Use Hashed for clientSettings * Send log notifications to client about session * Show test output directly * Add tests over client settings * Apply hlint hints * Simplify iface test to make it more robust Following @pepeiborra advise * Send session notifications only in test mode * Retry bench execution
… them (haskell/ghcide#731) * Store client settings in ide state * Log ide config registered in initHandler * Use a Maybe aware updater function * Create a Rule to get client settings * Create a specific getter for client settings * Trim trailing whitespace * Use modifyVar to avoid race conditions * Add comment to GetClientSettings * Use defineEarlyCutOffNoFile for GetClientSettings * Restart shake on config changed * Use Hashed for clientSettings * Send log notifications to client about session * Show test output directly * Add tests over client settings * Apply hlint hints * Simplify iface test to make it more robust Following @pepeiborra advise * Send session notifications only in test mode * Retry bench execution
… them (haskell/ghcide#731) * Store client settings in ide state * Log ide config registered in initHandler * Use a Maybe aware updater function * Create a Rule to get client settings * Create a specific getter for client settings * Trim trailing whitespace * Use modifyVar to avoid race conditions * Add comment to GetClientSettings * Use defineEarlyCutOffNoFile for GetClientSettings * Restart shake on config changed * Use Hashed for clientSettings * Send log notifications to client about session * Show test output directly * Add tests over client settings * Apply hlint hints * Simplify iface test to make it more robust Following @pepeiborra advise * Send session notifications only in test mode * Retry bench execution
3 participants
InitializeParamsand thedidChangeConfigurationParamsHandlernotificationonInitialConfigandonConfigChangecallbacks returns the same type (it is a type param in ghcide)alwaysRerun, inspired in Typecheck entire project on Initial Load and typecheck reverse dependencies of a file on saving #688. There @pepeiborra advised it can degrade performance, but i guess there will be not much configuration changes.GetClientSettings(for example the hlint rule that produces hlint diagnostics) is rerun on a file modification or opening a new one. Ideally i would like to trigger it within the configuration change itself (maybe restarting the shake session?)Sorry i am missing something obvious in the ghcide or shake concepts, this is my first pr touching their internals.