-
Notifications
You must be signed in to change notification settings - Fork 676
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
Provide more granular file change events based on VS Code file create/delete events #1805
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Except, I think the property name in the protocol is incorrect.
src/features/changeForwarding.ts
Outdated
let d1 = watcher.onDidCreate(onFileSystemEvent(FileChangeType.Create)); | ||
// In theory we don't need to subscribe to "change" notifications | ||
// because we already get them through the buffer update request | ||
//let d2 = watcher.onDidChange(onFileChange); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this to be last so that there isn't a gap between d1
and d3
?
src/omnisharp/protocol.ts
Outdated
@@ -417,6 +417,17 @@ export interface PackageDependency { | |||
Name: string; | |||
Version: string; | |||
} | |||
|
|||
export interface FilesChangedRequest extends Request{ | |||
ChangeType: FileChangeType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You called this FileChangeType
in your omnisharp-roslyn
PR. So this probably doesn't actually work. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the member name in the other PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'll wait until we update to a new OmniSharp release before merging.
@rchande: Out of curiosity, have you tested that this works? I'm curious since the protocol didn't match omnisharp-roslyn at first. |
@DustinCampbell I did test this--verified in the debugger that we were getting hit for both create and delete events. |
…/delete events