Skip to content

Conversation

michaelpj
Copy link
Collaborator

DRAFT: depends on unreleased lsp code, I expect I still messed some stuff up with the tests

This has a few substantive changes and a lot of messing with tests.

  • We now tell lsp our config section, and parse just that section.
  • We move the logic for updating the shake build rules for client config from a workspace/didChangeConfiguration handler to the new lsp callback, which will ensure it gets called in all circumstances that can be relevant.

The test changes are more annoying:

  • We ignore config and logging messages by default now, so we have to stop doing that when we care about it.
  • Many tests didn't really need to change the config, but rather just to set it once at the beginning. I adjusted a lot of test functions to allow passing in the initial config for this reason.

@michaelpj
Copy link
Collaborator Author

Stack builds will fail since I haven't put the git dep in there. Shouldn't be a problem once we're using a released lsp.

@July541
Copy link
Collaborator

July541 commented Aug 24, 2023

It looks like you are just going to satisfy the lsp changes, LGTM.

This has a few substantive changes and a lot of messing with tests.

- We now tell `lsp` our config section, and parse just that section.
- We move the logic for updating the shake build rules for client config
  from a `workspace/didChangeConfiguration` handler to the new `lsp`
  callback, which will ensure it gets called in all circumstances that
  can be relevant.

The test changes are more annoying:
- We ignore config and logging messages by default now, so we have to
  stop doing that when we care about it.
- Many tests didn't really need to _change_ the config, but rather just
  to set it once at the beginning. I adjusted a lot of test functions to
  allow passing in the initial config for this reason.
@michaelpj michaelpj requested a review from uhbif19 as a code owner August 25, 2023 08:45
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelpj michaelpj enabled auto-merge (squash) August 25, 2023 13:53
@michaelpj michaelpj merged commit e4234a3 into master Aug 25, 2023
@fendor fendor mentioned this pull request Aug 25, 2023
19 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants