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 sub-sub-directory support for ignored_folder #21

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shasderias
Copy link
Contributor

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
Copy link
Owner

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

This commit also "fixes" a subtle "bug" with the previous implementation
of isIgnoredFolder. The previous implementation tested whether a folder
is ignored by (1) splitting the path to be tested using strings.Split with
the seperator "/" and (2) testing whether the first part of the path exists in
w.IgnoredFolders. However, this does not work on architectures that use
a different path seperator. In practice, this "bug" does not present an
issue as an ignored sub-directory is skipped entirely by filepath.Walk.
@shasderias shasderias force-pushed the feature-ignore-sub-directory branch from 82e73bb to 2626995 Compare February 20, 2018 14:27
@shasderias
Copy link
Contributor Author

@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.

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.

2 participants