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

Support watching file extensions. #1053

Merged

Conversation

bjorkstromm
Copy link
Member

This will allow possibility to dynamically add .csx and .cake files to workspace.

Separate PR's for using this in CakeProjectSystem and ScriptProjectSystem is coming soon.

@@ -9,8 +9,8 @@ public interface IFileSystemWatcher
/// <summary>
/// Call to watch a file or directory path for changes.
/// </summary>
/// <param name="fileOrDirectoryPath">The file or directory path to watch.</param>
/// <param name="pathOrExtension">The file or directory path to watch.</param>
Copy link

@rchande rchande Dec 11, 2017

Choose a reason for hiding this comment

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

Previously, you could watch for files of any extension in a given directory. With this change, you can watch files of a given extension in any directory. However, there's still no way to watch files of a specific extension in a specific directory, and no way to watch all files in across all directories that omnisharp receives notifications about.

Maybe we should instead add an API that lets you subscribe to all notifications across all directories and let event subscribers handle filtering on extension/change type. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

there is a general need for globbing patterns support in OmniSharp - for example in case you define excluded paths in VS Code configuration, at the moment, OmniSharp doesn't know about that and still scans everything. I think what you suggest here could be developed together with a general globbing support in OmniSharp but personally I wouldn't hold up on this PR because it unblocks a very nice, and very welcome functionality that we've been missing for a while - watching for script and cake files

Copy link
Member Author

Choose a reason for hiding this comment

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

@rchande @filipw I’m fine either way. I actually started out with a glob pattern matchet before @rchande pushed the improvements to FileSystemWatcher, but never really finishef since I never made friends with Microsoft.Extensions.Filesystem.Globbing. After discussion with @DustinCampbell in Slack we came tovthe conclusion that a simple WatchExtension would be sufficient to support wath we currently need for Scripting and Cake. I’ll let you decide on how to move forward and adjust this PR accordingly 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

nvm the typos. It’s just me having a hard time texting on an iPhone..

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

LGTM

@david-driscoll david-driscoll merged commit 077c1f3 into OmniSharp:master Dec 12, 2017
@bjorkstromm bjorkstromm deleted the feature/watch-file-extension branch December 12, 2017 15:42
@DustinCampbell
Copy link
Contributor

Was the request for changes on this PR resolved before merging?

@bjorkstromm
Copy link
Member Author

@DustinCampbell well @rchande's original request to support watching every file was not implemented as @filipw thought this was ok for now and we should add glob-support instead in a future PR.

@DustinCampbell
Copy link
Contributor

No problem. I just wanted to check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants