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

Anchor excludes to start of string. Fixes #44. #46

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

Conversation

xaviershay
Copy link

Before this change, excluding a directory (bin) would also exclude posts
that include that name (scrubbing), which is very unexpected.

While this may start listening to files that were previously ignored, it
more closely matches the behaviour of jekyll on it's initial build
(which would include scrubbing). The consequences of being overly
aggressive on listening to changes seem less important than missing
something.

This change is a bandaid. A proper fix would be to unify with Jekyll's
exclusion logic (Jekyll::EntryFilter maybe?) to ensure they will always
be the same. In the meantime, it may be advisable to not include
Jekyll's exclude config in the watchers ignore list and take the hit
of overly aggressive reloads. I leave that decision to someone more
experienced than I.

Before this change, excluding a directory (`bin`) would also exclude posts
that include that name (`scrubbing`), which is very unexpected.

While this may start listening to files that were previously ignored, it
more closely matches the behaviour of jekyll on it's initial build
(which _would_ include `scrubbing`).  The consequences of being overly
aggressive on listening to changes seem less important than missing
something.

This change is a bandaid. A proper fix would be to unify with Jekyll's
exclusion logic (Jekyll::EntryFilter maybe?) to ensure they will always
be the same. In the meantime, it _may_ be advisable to not include
Jekyll's `exclude` config in the watchers ignore list and take the hit
of overly aggressive reloads. I leave that decision to someone more
experienced than I.
@parkr
Copy link
Member

parkr commented Jan 20, 2017

Can you please add a test case where you assert that a file is left alone that you intend to leave alone?

@xaviershay
Copy link
Author

yeah I'll have a go, might not be until the weekend though

@xaviershay
Copy link
Author

Please take a look now. There wasn't any prior art for this so I had to make something up. I changed the existing adjacent spec to be consistent with new method, though didn't change the entire file. (In general it does a lot of "check the config is set to what I wanted" rather than "does this config achieve the behaviour I want").

@DirtyF DirtyF requested a review from a team October 23, 2017 10:41
@parkr
Copy link
Member

parkr commented Nov 3, 2017

Yeesh, we really let this sit here a while. Sorry about that!

My initial hesitation (which is maybe why I didn't respond) is how this affects files in a subdirectory. If I want to ignore LICENSE, then I want to ignore ./LICENSE and ./site/LICENSE etc. The tests didn't help me see whether that works.

If you're still interested in this PR, I'd gladly accept it if you can clarify my question above ☝️ . Thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants