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

Recover Directory Paths #29

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

Recover Directory Paths #29

wants to merge 5 commits into from

Conversation

danprince
Copy link

Started having problems with naming my SCSS components site-*, as they are required to begin with a leading _. Obviously, this was getting caught in the ignored files for Listen.

The process of creating a relative path from an absolute one seems to strip the trailing slash, identifying a path as a directory not a file. So when this is converted into a Regex for adding to the ignore list, it also ignores files that start with the same name as ignored directories.

This fix simply checks whether there was originally a trailing slash, then adds it again before it is converted into a regular expression. Not the most elegant solution, but it works for now (until there's a more fine grained alternative in place anyway). Fixes #22.

@parkr
Copy link
Member

parkr commented Nov 24, 2015

I see the issue, great idea to fix it. Could you take a look at the failing tests and try to patch them?

@danprince
Copy link
Author

Not exactly sure what's going on with the Gem::InstallError on the 1.93 tests. Jekyll's minimum Ruby version got updated since the travis test versions were specified?

Haven't ever contributed to a ruby project before, so not super sure of the ecosystem. Can I just knock the 1.9.3 version out of travis.yml?

@parkr
Copy link
Member

parkr commented Nov 24, 2015

@danprince Fixed in #30. Would it be possible for you to rebase on origin/master?

danprince added 3 commits November 25, 2015 10:12
The process of creating a relative path from an absolute one seems to
strip the trailing slash, identifying a path as a directory not a file.

When this is converted into a Regex for adding to the ignore list, it
also ignores files that start with the same name as ignored directories.

This fix simply checks whether there was originally a trailing slash,
then adds it again before it is converted into a regular expression.
@danprince
Copy link
Author

Attempted the rebase, but the 1.93 version test is still there and failing for some reason. Can't understand why though, the output looks ok.

@parkr
Copy link
Member

parkr commented Nov 27, 2015

Attempted the rebase, but the 1.93 version test is still there and failing for some reason. Can't understand why though, the output looks ok.

Looks like some incompat with Rspec and equality operators? @envygeeks, have you seen this before?

@envygeeks
Copy link
Contributor

It normally happens when a hash isn't in the order you tested it in, I'll send up a pull to fix it.

@envygeeks
Copy link
Contributor

I'm dumb, I just went to fix this problem and looked at the source and the error and it just dawned on me what happened. You are using Regexp.escape, in 1.9 the sources will mismatch and #==, #eql? will fail because the source will be "val/". That's your problem here, just to demonstrate I dropped myself into your context to give you an example:

[1] pry(#<*>)> $stdout.puts a.inspect, b.inspect
[/_config\.yml/, /_site\//, /\.jekyll\-metadata/]
[/_config\.yml/, /_site\//, /\.jekyll\-metadata/]

[2] pry(#<*>)> $stdout.puts a[1].source, b[1].source
_site/
_site\/

@envygeeks
Copy link
Contributor

Here is a patch for you @parkr @danprince:

diff --git a/spec/watcher_spec.rb b/spec/watcher_spec.rb
index 1c6a32a..14202f5 100644
--- a/spec/watcher_spec.rb
+++ b/spec/watcher_spec.rb
@@ -10,7 +10,7 @@ describe(Jekyll::Watcher) do

   let(:options) { base_opts }
   let(:site)    { instance_double(Jekyll::Site) }
-  let(:default_ignored) { [/_config\.yml/, /_site\//, /\.jekyll\-metadata/] }
+  let(:default_ignored) { [/_config\.yml/, /#{Regexp.escape("_site/")}/, /\.jekyll\-metadata/] }
   subject { described_class }
   before(:each) do
     FileUtils.mkdir(options['destination']) if options['destination']
@@ -88,7 +88,7 @@ describe(Jekyll::Watcher) do
     end

     context "with a custom destination" do
-      let(:default_ignored) { [/_config\.yml/, /_dest\//, /\.jekyll\-metadata/] }
+      let(:default_ignored) { [/_config\.yml/, /#{Regexp.escape("_dest/")}/, /\.jekyll\-metadata/] }

       context "when source is absolute" do
         context "when destination is absolute" do

@danprince
Copy link
Author

@parkr @envygeeks Got it, cheers.

@ckreon

This comment has been minimized.

@alenvuletic

This comment has been minimized.

@pathawks
Copy link
Member

/cc: @jekyll/build

@mattr- mattr- self-assigned this Jun 28, 2017
@DirtyF DirtyF requested review from mattr- and a team and removed request for mattr- December 2, 2017 21:33
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.

Changes to files starting with _site are not being picked up
8 participants