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

Add API for registering an external file watcher implementation #1764

Merged
merged 19 commits into from
Jul 3, 2021

Conversation

rchl
Copy link
Member

@rchl rchl commented Jun 26, 2021

Allows to create external package that implements the file watcher API.

See example implementation at sublimelsp/LSP-file-watcher-chokidar#1

plugin/core/sessions.py Outdated Show resolved Hide resolved
from abc import abstractmethod


DEFAULT_IGNORES = ['**/.git/**', '**/node_modules/**', '**/.hg/**']
Copy link
Member

Choose a reason for hiding this comment

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

Would be better if this were binary_file_patterns and/or index_exclude_patterns. But the glob syntax/features of ST is unfortunately incomplete compared to VSCode.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't you need **/.svn/** here as well?

Copy link
Member Author

@rchl rchl Jul 3, 2021

Choose a reason for hiding this comment

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

Someone is using SVN still? :)

I can add of course but was planning to look into fetching ignores from ST and converting to glob but that might turn out to be some work.

Copy link
Member

Choose a reason for hiding this comment

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

Someone is using SVN still? :)

Unfortunately, yes, these companies exist ;) But I think it's okay to forget about it and focus on fetching the ignore list from ST instead.

@rchl
Copy link
Member Author

rchl commented Jun 28, 2021

On the high-level this seems fine?

I'm open to nitpicking too. :)

@rwols
Copy link
Member

rwols commented Jun 28, 2021

I’m quite busy with IRL responsibilities, will review maybe Thursday or Friday. But one thing I do miss is some sort of integration test with the test/server.py file.

@predragnikolic
Copy link
Member

On the high-level this seems fine?

Yes, the api looks nice (not to many methods exposed and the one that are exposed are reasonably easy to understand) 🙂

@rchl rchl marked this pull request as ready for review June 29, 2021 21:55
@rchl
Copy link
Member Author

rchl commented Jun 29, 2021

I'll say it's ready for review although would be good to also handle ST's exclude patterns. Maybe it wouldn't be too hard to convert those to globs.

@rchl
Copy link
Member Author

rchl commented Jun 29, 2021

Note:

  • changes in sessions.py are much more readable with "hide whitespace changes" enabled. Had to extract each capability group into a variable to avoid type issue.
  • removed the on_register_capability_async and on_unregister_capability_async APIs which to my knowledge were only used by me in my previous file watcher implementation.
  • The watchers are only registered if there are workspace folders. I think something else wouldn't make sense.
  • I think I'm not handling workspace folder changes. Should reset/recreate the watchers then.

plugin/core/sessions.py Outdated Show resolved Hide resolved
plugin/core/sessions.py Outdated Show resolved Hide resolved
sublime-package.json Outdated Show resolved Hide resolved
Copy link
Member

@rwols rwols left a comment

Choose a reason for hiding this comment

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

Looks like you've put a lot of work into this.

@rwols
Copy link
Member

rwols commented Jul 3, 2021

Let's go ahead with this for now and work out the kinks.

@rwols rwols merged commit 4b4f9fe into main Jul 3, 2021
@rwols rwols deleted the feat/watcher-api branch July 3, 2021 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants