Skip to content

Add ability for plugins to handle file change notifications #1588

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Mar 20, 2021
Merged

Add ability for plugins to handle file change notifications #1588

merged 17 commits into from
Mar 20, 2021

Conversation

pepeiborra
Copy link
Collaborator

Rebased #1585 and extracted ghcide notification handlers

@pepeiborra pepeiborra requested a review from alanz March 17, 2021 18:59
alanz
alanz reviewed Mar 17, 2021
View reviewed changes
Copy link
Collaborator

@alanz alanz left a comment

Choose a reason for hiding this comment

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

There seems to be some duplication between the HLS and ghcide side.

Otherwise seems to be what I had before.

@pepeiborra pepeiborra force-pushed the plugin-notifications branch from f0adfb8 to 85cb54c Compare March 17, 2021 21:30
@isovector
Copy link
Collaborator

isovector commented Mar 18, 2021 •
edited
Loading

Is this capable of watching arbitrary directories on the filesystem? I wanted such a thing in #524, in order to watch $XDG_CONFIG/wingman.

@pepeiborra
Copy link
Collaborator Author

Is this capable of watching arbitrary directories on the filesystem? I wanted such a thing in #524, in order to watch $XDG_CONFIG/wingman.

No, only workspace folders. Outside the workspace, you would need to implement your own watching

@pepeiborra pepeiborra force-pushed the plugin-notifications branch from 92e4a85 to 0f22329 Compare March 18, 2021 08:46
@jneira
Copy link
Member

jneira commented Mar 18, 2021

Is this capable of watching arbitrary directories on the filesystem? I wanted such a thing in #524, in order to watch $XDG_CONFIG/wingman.

No, only workspace folders. Outside the workspace, you would need to implement your own watching

I would need it for hlint config files for #594, maybe it worths to implement it in some common place (hls-plugin-api?)

@pepeiborra pepeiborra marked this pull request as ready for review March 18, 2021 14:10
@pepeiborra
Copy link
Collaborator Author

Is this capable of watching arbitrary directories on the filesystem? I wanted such a thing in #524, in order to watch $XDG_CONFIG/wingman.

No, only workspace folders. Outside the workspace, you would need to implement your own watching

I would need it for hlint config files for #594, maybe it worths to implement it in some common place (hls-plugin-api?)

Rather than turning HLS into an operating system, I would push back on these feature requests and ask people to reload

@pepeiborra pepeiborra force-pushed the plugin-notifications branch from 071074e to 60e36d1 Compare March 18, 2021 15:26
@pepeiborra pepeiborra requested a review from alanz March 18, 2021 18:36
@pepeiborra pepeiborra force-pushed the plugin-notifications branch from 874c4c2 to 8821bba Compare March 18, 2021 19:07
alanz and others added 14 commits March 19, 2021 08:58
@alanz @pepeiborra
Provide pluginNotificationhandlers too
9ade5e5 
Like pluginHandlers, but for notifications

At present the last one in the chain wins, so if one is set it
overrides the one built into ghcide
@pepeiborra
Fix handling of config
f334ee5
@pepeiborra
run the handlers in parallel
b580c38
@pepeiborra
add missing instances
ebdd7af
@pepeiborra
Extract ghcide notification handlers to an HLS plugin
9618fa1 
This is required to allow for user defined notification handlers, otherwise
HLS plugins will overwrite the ghcide handlers and nothing will work
@pepeiborra @wz1000
Update hls-plugin-api/src/Ide/Types.hs
3c1d1e9 
Co-authored-by: wz1000 <zubin.duggal@gmail.com>
@pepeiborra
bump version numbers to track breaking changes
babbb93
@pepeiborra
hlint pragma
36ca256
@pepeiborra
fixup! Update hls-plugin-api/src/Ide/Types.hs
0b312fc
@pepeiborra
relax depends constraints
6c59330
@pepeiborra
redundant import
96c2647
@pepeiborra
fixup! Update hls-plugin-api/src/Ide/Types.hs
97e0476
@pepeiborra
clean up
cf8ea51
@pepeiborra
run notification handlers sequentially
352272d
@pepeiborra pepeiborra force-pushed the plugin-notifications branch from 8821bba to 352272d Compare March 19, 2021 08:58
@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Mar 20, 2021
@mergify mergify bot merged commit b9c6e6c into master Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanz alanz alanz approved these changes

@wz1000 wz1000 wz1000 approved these changes

Assignees
No one assigned
Labels
merge me Label to trigger pull request merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pepeiborra @isovector @jneira @alanz @wz1000