search

markbates / refresh

MIT License
191 stars 30 forks source link

add sub-sub-directory support for ignored_folder #21

Open shasderias opened 6 years ago

shasderias commented 6 years ago

This pull request adds support for ignoring sub-sub-folders in ignored_folders.

Use-case: refresh.yml

ignored_folders:
- cmd/web/client

Where cmd/web/client contains front-end code (JS, CSS etc.) which should be ignored, but not cmd/web which contains back-end code (.go files).

Ended up implementing this as refresh was filepath.Walk-ing cmd/web/client/node_modules and killing my CPU even though I had cmd/web/client on my ignored_folders list. In the process of doing so, I think I came across a subtle bug with the original isIgnoredFolder function which I took the opportunity to document in the commit message. If I have misunderstood the bug, please amend the commit message before committing.

Pull request also includes a simple test for isIgnoredFolder.

Please feel free to amend/rework these commits as you see fit.

Thanks for the project!

markbates commented 6 years ago

@shasderias it looks like the tests are failing for some reason on travis

shasderias commented 6 years ago

@markbates The path returned by filepath.Walk uses path separators appropriate to the platform the program is being run on. On Windows systems, filepath.Rel handles both Windows and POSIX path separators correctly, but on POSIX systems, the paths passed to filepath.Rel are expected to use POSIX style path separators.

My test cases included paths with both Windows and POSIX style path separators and this caused the tests to fail on Linux systems. I have split the test cases in watcher_test.go into two separate files, watcher_windows_test.go and watcher_posix_test.go with the appropriate build tags ('+build windows' and '+build linux darwin' respectively). Tests now pass on Windows, Linux (WSL Ubuntu) and Darwin.

Additionally, this also means for cross-platform compatibility, ignored_paths paths in refresh.yml should use POSIX-style separators. I have documented this in the readme.

I should have been more careful, sorry for the noise.

Please take another look.