Cleaner: `keep_files` should only apply to the beginning of paths, not substrings with index > 0 #3849

Merged
merged 5 commits into from Mar 24, 2016

Conversation

Projects
None yet
4 participants
@shinnkondo
Contributor

shinnkondo commented Jul 14, 2015

keep_files could prevent more files than intended from being removed.

For example, if someone is working under a folder something like username.github.io, keep_files: [".git", ".svn"] matches any files under username.github.io/_sites, leaving the destination folder uncleaned.

This would be confusing when a file is renamed while it is viewed on a browser. An user can be unaware of the fact that he is looking at outdated file (see #3847).

According to the documentation, the paths in keep_files should be "relative to the destination" rather than being keywords.

For these reasons, I made a change so that keep_files are interpreted as paths under the destination folder.

shinnkondo added some commits Jul 13, 2015

Added a new case for test_clearner
where a directory is not in keep_files, but its path contains a string in keep_files.
Fix keep_files to be used as paths relative to the destination.
They were used as keywords to match files containing them in the paths.

@shinnkondo shinnkondo changed the title from keep_files should be read as paths under destination rather than keywords. to keep_files should be read as paths under destination rather than keywords for cleaner to work. Jul 18, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Sep 1, 2015

Contributor

What is the status of this?

Contributor

envygeeks commented Sep 1, 2015

What is the status of this?

@parkr parkr added needs-work and removed pending-feedback labels Dec 16, 2015

@parkr parkr self-assigned this Dec 16, 2015

@jekyllbot jekyllbot closed this Jan 14, 2016

@parkr parkr reopened this Jan 14, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 23, 2016

Member

For example, if someone is working under a folder something like username.github.io, keep_files: [".git", ".svn"] matches any files under username.github.io/_sites, leaving the destination folder uncleaned.

The example here doesn't apply to the feature. keep_files is a way of inhibiting the removal of files and folders inside of your compiled site directory (e.g. _site). In your example, it sounds like you think of keep_files as a way to keep your site's source from being cleaned up.

Am I misreading you here?

Member

parkr commented Mar 23, 2016

For example, if someone is working under a folder something like username.github.io, keep_files: [".git", ".svn"] matches any files under username.github.io/_sites, leaving the destination folder uncleaned.

The example here doesn't apply to the feature. keep_files is a way of inhibiting the removal of files and folders inside of your compiled site directory (e.g. _site). In your example, it sounds like you think of keep_files as a way to keep your site's source from being cleaned up.

Am I misreading you here?

@shinnkondo

This comment has been minimized.

Show comment
Hide comment
@shinnkondo

shinnkondo Mar 23, 2016

Contributor

First, thank you for your feedback.

I am afraid you are. I think of keep_files as a way to keep a compiled site directory, not source.
If you would take a look at #3847, you might get some ideas about my intent; I was confused by the fact that compiled outdated html was not removed.

Let me know if you are still unsure about my post. I know I am not a good writer.

Contributor

shinnkondo commented Mar 23, 2016

First, thank you for your feedback.

I am afraid you are. I think of keep_files as a way to keep a compiled site directory, not source.
If you would take a look at #3847, you might get some ideas about my intent; I was confused by the fact that compiled outdated html was not removed.

Let me know if you are still unsure about my post. I know I am not a good writer.

test/test_cleaner.rb
+
+ teardown do
+ FileUtils.rm_rf(dest_dir('.git'))
+ FileUtils.rm_rf(dest_dir('.username.github.io'))

This comment has been minimized.

@parkr

parkr Mar 23, 2016

Member

This should not include the preceding ., correct?

@parkr

parkr Mar 23, 2016

Member

This should not include the preceding ., correct?

This comment has been minimized.

@shinnkondo

shinnkondo Mar 23, 2016

Contributor

That is correct. Thank you for pointing it out.

@shinnkondo

shinnkondo Mar 23, 2016

Contributor

That is correct. Thank you for pointing it out.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 23, 2016

Member

@shinkondo I see, thank you for your patience.

my blog folder name containing ".git" as a substring

This seems to have explained it for me. You have keep_files: ".git" with a subdirectory of username.github.io and you find that this subdirectory, once created, is not cleaned?

I am 👍 on this PR. Just one comment above.

Member

parkr commented Mar 23, 2016

@shinkondo I see, thank you for your patience.

my blog folder name containing ".git" as a substring

This seems to have explained it for me. You have keep_files: ".git" with a subdirectory of username.github.io and you find that this subdirectory, once created, is not cleaned?

I am 👍 on this PR. Just one comment above.

@parkr parkr added this to the 3.2 milestone Mar 23, 2016

@shinnkondo

This comment has been minimized.

Show comment
Hide comment
@shinnkondo

shinnkondo Mar 24, 2016

Contributor

I think what you described captures my issue, but please don't pull this request as of now.

It seems to me that the situation has changed because of bd2c337. My code in pull-request would not work. Now regex generated from keep_files would match against relative paths under _sites, not absolute paths. My code will generate regex with absolute paths. I bet it would not match anything practically.

Contributor

shinnkondo commented Mar 24, 2016

I think what you described captures my issue, but please don't pull this request as of now.

It seems to me that the situation has changed because of bd2c337. My code in pull-request would not work. Now regex generated from keep_files would match against relative paths under _sites, not absolute paths. My code will generate regex with absolute paths. I bet it would not match anything practically.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 24, 2016

Member

@shinkondo You're welcome to merge origin/master into this branch and let Travis CI take a crack at it. bd2c337 doesn't fix your issue, though, does it?

Member

parkr commented Mar 24, 2016

@shinkondo You're welcome to merge origin/master into this branch and let Travis CI take a crack at it. bd2c337 doesn't fix your issue, though, does it?

@shinnkondo

This comment has been minimized.

Show comment
Hide comment
@shinnkondo

shinnkondo Mar 24, 2016

Contributor

No, bd2c337 does not fix my issue.

As Travis CI showed, I was wrong to say that my code would not work. It seems that the code is using absolute paths again because of bd2c337.

In any case, I can fix my mistake in the test. Is there anything else I should do? I can try to squash/rebase the git log if that is preferable.

Contributor

shinnkondo commented Mar 24, 2016

No, bd2c337 does not fix my issue.

As Travis CI showed, I was wrong to say that my code would not work. It seems that the code is using absolute paths again because of bd2c337.

In any case, I can fix my mistake in the test. Is there anything else I should do? I can try to squash/rebase the git log if that is preferable.

@parkr parkr changed the title from keep_files should be read as paths under destination rather than keywords for cleaner to work. to Cleaner: `keep_files` should only apply to the beginning of paths, not substrings with index > 0 Mar 24, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 24, 2016

Member

This looks just fine to me. Thank you!

@jekyllbot: merge +bug

Member

parkr commented Mar 24, 2016

This looks just fine to me. Thank you!

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 7a4817e into jekyll:master Mar 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Mar 24, 2016

parkr added a commit that referenced this pull request Mar 26, 2016

Merge remote-tracking branch 'origin/master' into themes
* origin/master: (65 commits)
  Update history to reflect merge of #4703 [ci skip]
  Update history to reflect merge of #4712 [ci skip]
  Highlight the test code
  Update history to reflect merge of #4640 [ci skip]
  readded "env=prod"-condition
  Update history to reflect merge of #3849 [ci skip]
  Update history to reflect merge of #4624 [ci skip]
  Update history to reflect merge of #4704 [ci skip]
  Update history to reflect merge of #4706 [ci skip]
  Checks for link file extension in tests
  Updating assets documentation
  Fix test teardown for cleaner.
  Update history to reflect merge of #4542 [ci skip]
  Add explanation of site variables in the example _config.yml
  Use double quotes in the gemfile
  Add test for creation of Gemfile by 'jekyll new'
  Add comment about github-pages
  Update history to reflect merge of #4533 [ci skip]
  Ensure Rouge closes its div/figure properly after highlighting ends.
  Add Site#config= which can be used to set the config
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment