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 watch_exclude setting to _config.yml #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mohkale
Copy link

@mohkale mohkale commented Dec 28, 2019

see issue #90

@mohkale mohkale force-pushed the master branch 2 times, most recently from 7e4c188 to 2eef38d Compare December 28, 2019 15:46
@ashmaroli
Copy link
Member

The method :listen_ignore_paths already returns an array of regular expressions. I think it would be better to enhance that method to just convert glob_patterns to regex as well (like the author noted in the issue ticket) instead of requiring a new config option (may result in confusion due to overlap).

@mohkale
Copy link
Author

mohkale commented Dec 29, 2019

@ashmaroli Understandable, but I don't think it'd be very reliable. What about the annoying edge cases where a person supplies perfectly valid fnmatch arguments, but the regex it produces backtracks and then just hangs forever (I'm not sure whether there's a way to detect and prevent that). Honestly I think jekyll should've made exclude use regexps from the start... but it didn't and now we have to live with the incongruency.

Another, slightly nicer option, would be to add an option to the listen library where it can take a callable for :ignore and then have that callable use fnmatch behind the scenes.

If all that's impossible, then we do still have the option of simply using fnmatch in our listen handler. so in the listen_handler method, we filter out any of the paths in modified, added & removed that match any of our exclude patterns. If all three lists are then empty, exit the method early.

I still think a new config option is the nicest way to do this, but that last approach is the second best :).

@ashmaroli
Copy link
Member

I like the second-best option more.. Will you be able to submit a separate PR towards that idea..?

@mohkale
Copy link
Author

mohkale commented Dec 29, 2019

Yep, I'll submit sometime later today 😄.

Do you want me to remove the watch_exclude config option then?

@ashmaroli
Copy link
Member

Lets leave this PR as it is..
The second-best option is to be a separate PR. If that gets accepted by other maintainers, then this PR (and the watch_exclude idea) can be discarded,

@mohkale
Copy link
Author

mohkale commented Dec 30, 2019

Sorry, got distracted with another project. Anyways see #92.

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