8000 Cleaner: `keep_files` should only apply to the beginning of paths, not substrings with index > 0 by shinnkondo · Pull Request #3849 · jekyll/jekyll · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Cl 8000 eaner: keep_files should only apply to the beginning of paths, not substrings with index > 0 #3849

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

Merged
merged 5 commits into from
Mar 24, 2016

Conversation

shinnkondo
Copy link
Contributor

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.

where a directory is not in keep_files, but its path contains a string in keep_files.
They were used as keywords to match files containing them in the paths.
@shinnkondo shinnkondo changed the title keep_files should be read as paths under destination rather than keywords. keep_files should be read as paths under destination rather than keywords for cleaner to work. Jul 18, 2015
@envygeeks
Copy link
Contributor

What is the status of this?

@envygeeks envygeeks added the pending-feedback We are waiting for more info. label Sep 1, 2015
@parkr parkr added needs-work and removed pending-feedback We are waiting for more info. 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
Copy link
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?

@parkr parkr added the pending-feedback We are waiting for more info. label Mar 23, 2016
@shinnkondo
Copy link
Contributor Author

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.


teardown do
FileUtils.rm_rf(dest_dir('.git'))
FileUtils.rm_rf(dest_dir('.username.github.io'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not include the preceding ., correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. Thank you for pointing it out.

@parkr
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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 keep_files should be read as paths under destination rather than keywords for cleaner to work. Cleaner: keep_files should only apply to the beginning of paths, not substrings with index > 0 Mar 24, 2016
@parkr
Copy link
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
jekyllbot added a commit that referenced this pull request Mar 24, 2016
@parkr parkr removed needs-work pending-feedback We are waiting for more info. labels Mar 24, 2016
parkr added a commit that referenced this pull request Mar 26, 2016
* 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
  ...
@jekyll jekyll locked and limited conversation to collaborators Jun 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0