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

Use file watches for all workspace files #1880

Merged
merged 4 commits into from
Jun 1, 2021
Merged

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented May 31, 2021

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
-- 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
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
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
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.

None yet

2 participants