Skip to content

Use file watches for all workspace files#1880

Merged
mergify[bot] merged 4 commits into
masterfrom
file-watches-for-all
Jun 1, 2021
Merged

Use file watches for all workspace files#1880
mergify[bot] merged 4 commits into
masterfrom
file-watches-for-all

Conversation

@pepeiborra

@pepeiborra pepeiborra commented May 31, 2021

Copy link
Copy Markdown
Collaborator

This PR hooks up the GetModificationTime rule to install new file watchers for workspace files that are not already covered by the initial glob that we install, consolidating the tracking of workspace changes. There are two main use cases for this:

  1. Cradle dependencies, including the hie.yaml file and the cabal/stack descriptors
  2. TH dependencies, defined using addDependentFile.

The implementation adds a new zero-dependency rule AddWatchedFile that creates the file watches, and makes GetModificationTime depend on it. The rest of the changes are mostly refactorings needed to hook things up.

The motivation for this change is not just consolidation or performance. This is required in #1862 to be able to track changes in the use cases listed above.

@pepeiborra
pepeiborra requested a review from wz1000 May 31, 2021 23:04
@pepeiborra
pepeiborra force-pushed the file-watches-for-all branch from 6720435 to 9a782b7 Compare May 31, 2021 23:06
-- support that: https://github.com/bubba/lsp-test/issues/77
watchers = [ watcher (Text.pack glob) | glob <- globs ]

void $ LSP.sendRequest LSP.SClientRegisterCapability regParams (const $ pure ()) -- TODO handle response

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ideally we would block waiting for the response return and return a result depending on whether it was successful.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is an old TODO, it's not something added in this PR. I agree that we should wait for the response, but maybe not for this PR

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

BTW I checked this and lsp-test doesn't seem to implement this part of the protocol, opened haskell/lsp#333

@pepeiborra
pepeiborra marked this pull request as ready for review June 1, 2021 10:27
@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge me Label to trigger pull request merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants