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

Implement project.watch configuration options #1270

Conversation

matuszeman
Copy link

What this PR does / why we need it:

TODO

Which issue(s) this PR fixes:

Fixes #1269

TODO

@matuszeman matuszeman changed the title WIP Implement prototype of project.watch config Implement project.watch configuration options Oct 17, 2019
@eysi09
Copy link
Collaborator

eysi09 commented Oct 23, 2019

Hi @matuszeman!

Let me start by apologising for the late response. And follow up with a big thank you for the pull request!

The person you'd been in contact with is away on vacation so this one somehow fell in between the cracks. I'm really sorry for that.

I'm away myself until after the weekend but we'll get to this ASAP early next week.

@eysi09 eysi09 requested a review from edvald October 29, 2019 10:43
@edvald
Copy link
Collaborator

edvald commented Oct 30, 2019

Sorry I just got to this, back from vacation now. I think this is a good feature to add, but there's a bit of nuance to consider in the implementation.

A couple of points I see at first glance:

  1. It might actually make sense to just use the modules.include/modules.exclude fields. We're only really watching for changes to modules, and the separate field is likely superfluous.
  2. We can only in some cases apply the modules.include field as an optimization, which is when there are only specific directories listed under that field, or directories followed by globs. That's feasible to implement, but would need to be documented.
  3. The exclude part should work mostly as-is, just needs a bit more work to handle configuration changes. We essentially will need to fully reset the watcher when we reconfigure. Shouldn't be hard though, and likely better to do that either way.
  4. We would need to add some tests to make sure this all works as expected, but of course this is just a draft PR :)

@matuszeman would you like to carry on with this, or should I have a go?

@matuszeman
Copy link
Author

You came and I'm leaving until Sunday :) so to move things forward I'd appreciate any help from your side if you find spare second.

I've been also thinking about implementing watch option on module level instead where it might make more sense maybe, and will be closer to the codebase of particular module, thus easier to manage.
modules on project level would be used to discover module garden files only and watch for those and module configuration would dictate what to watch on module level only. One module could be with NodeJS codebase where I'd definitely exclude node_modules or PHP with its vendor folder for example.

@edvald
Copy link
Collaborator

edvald commented Nov 8, 2019

Closing in favor of #1320, but thanks so much @matuszeman for the contribution!

@edvald edvald closed this Nov 8, 2019
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.

File watching optimization
3 participants