guarantee jekyll-watch respects users exclude configuration

[mohkale]

by double-tapping (sly zombieland reference 😆) paths which
don't match to exact (existing) files.

this patch stores any of these non-absolute paths as wildcard paths;
which it then rechecks & then strips from paths in the response handler
for the listen package.

NOTE maybe it'd be better & more reliable to simply use fnmatch?
exclusively and not pass any filtering regexps to listen.

[DirtyF]

Thanks for you effort, tests will need to pass though.

[mohkale]

yeah, my bad, I'm on it now.

[mohkale]

kay, this should be ok I hope 😄.

[ashmaroli]

There's a lot happening here.. At a first glance, it looks like the maintainability of the code went down significantly while the complexity went proportionately higher..

Needs some refactoring.
Couple of tips:

• Always prefer [].flat_map over [].flatten.map
• If the last expression is a multi-line conditional block, use a guard clause:

  def foo
    ...
    unless something
      lorem ipsum
      dolor sit, amet
    end
  end

  can be simplified to

  def foo
    ...
    return if something
  
    lorem ipsum
    dolor sit, amet
  end

• When using a conditional with if..else..end, always place the positive condition first:

  if something
    foo
    bar
  else
    lorem
  end
  

• We used Array(options["exclude"]) to ensure that we always have an array. Using options.fetch('exclude', []) is not the equivalent. The latter will fail if exclude was set to a simple String.
• [].select { |i| i.find { ... }.nil? } feels like a hack, sorry.

[mohkale]

Always prefer [].flat_map over [].flatten.map

Did not know about that, changed.

If the last expression is a multi-line conditional block, use a guard clause:

I've been afraid to use return ever since I found out it also exits any containing functions if used in blocks. I suppose that's being overly paranoid.

changed in listen_ignore_paths (though I could swear it was always an unless/end expression) but kept the same in listen_handler because of the aforementioned jump errors.

When using a conditional with if..else..end, always place the positive condition first:

I thought it was easier to read this way, seeing as the false condition is a single expression... but understood. changed.

We used Array(options["exclude"]) to ensure that we always have an array. Using options.fetch('exclude', []) is not the equivalent. The latter will fail if exclude was set to a simple String.

didn't know exclude could be a simple string. changed and added section to docstring.

[].select { |i| i.find { ... }.nil? } feels like a hack, sorry.

Can't really tell how else to write it. That seemed like the most compact yet expressive way. If you have any suggestions please feel free to share. for now I've just added a comment elaborating on what it does.

Also no need for sorry, any other criticisms are greatly appreciated; I'm not used to professional ruby development and learning through practice is the best route. Let me know if there are any other issues.

[ashmaroli]

I've been afraid to use return ever since I found out it also exits any containing functions if used in blocks.

Yep. one can't return from inside a block (or a proc) since it will result in a LocalJumpError. But you can do so in a lambda. In a block, one uses guard clauses with a next.
The above is just a FYI. No need to implement here.

didn't know exclude could be a simple string..

Jekyll doesn't sanitize config values. If a user provides Jekyll with exclude: "src/js", it may result in an error even before jekyll-watch is loaded but the type-coercion already in place here is best kept as is.

Can't really tell how else to write it. That seemed like the most compact yet expressive way.

Compact, yes. Expressive, not quite. However, I don't have an alternative suggestion for now..

Let me know if there are any other issues

The following don't make sense to me:

• nil.tap { exclusion_fnmatch_paths << relative_path }
  Why not exclusion_fnmatch_paths << relative_path directly? Does this have test coverage?

---

Thank you for all the work on this branch and considering my requests promptly.

[mohkale]

The following don't make sense to me:

that's what I had at the beginning, but exclusion_fnmatch_paths << relative_path evaluates to the value of exclusion_fnmatch_paths, which means every time that expression evaluates the entire exclusion_fnmatch_paths array gets included in the result of the map block. Then when you call compact it all gets flattened and you get an array with a lot of duplicate variables that shouldn't be there to begin with.

Ideally I'd liked to have changed Watcher from a module to a class, that way I could just have two instance fields and then change the map block to an each block which appends to the correct field. but I thought that would've been too big a change to just do.