Support recursive globs in file monitoring (#10064)#11658
Support recursive globs in file monitoring (#10064)#11658mergify[bot] merged 1 commit intohaskell:masterfrom
Conversation
|
Will this resolve #10064? |
Yes, that is the idea. |
sheaf
left a comment
There was a problem hiding this comment.
This is looking great, but I think the testing strategy is insufficient. It's quite awkward to go through SetupHooks for testing; I'm not sure whether there's a better (and more direct) way to test the code. We definitely need to test more things:
- recurring deeper into subdirectories of subdirectories
- making sure that we don't monitor things that we shouldn't:
- adding a directory which contains no files that match
- modifying a file that doesn't match at any level of the directory hierarchy
Again it's not great to test this within the SetupHooks framework as it muddies the waters (and unrelated bugs may interfere).
I'm also wondering whether we need to take into account possible recursive symlinks?
If we have foo/bar/baz which is a symlink back to foo, then we could enter an infinite loop here. So perhaps we need to additionally keep track of which directories we have seen to avoid entering them again?
Probably also:
Indeed, I found this test quite awkward to write. Do you have any pointers for me where/how this could better fit into the existing testing framework?
Good point, we should probably do that. The alternative would be to just not support symlinks, but that doesn't sound like a nice thing to do to me. |
It seems there are unit tests for file monitoring in |
a59767e to
2a7632c
Compare
b97a1ca to
4fcdd0d
Compare
|
This looks excellent. As far as I can see, all that's left to do is to rebase, squash the commits, and update the expected output for the |
0f2d6d0 to
894f7a9
Compare
3c16525 to
53d2cda
Compare
Merge Queue Status
This pull request spent 10 minutes 25 seconds in the queue, including 1 second running CI. Required conditions to merge
|
Implements support for recursive globs (
some/path/**/file.name) for file monitoring.significance: significantin the changelog file.QA Notes
Previously, attempting to use recursive globs (
**) to declare build rules (with build-type "Hooks") would yield an error message saying that this isn't implemented; this should now work. See #10064 for details and examples.