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

clean_site default to preview = TRUE #1973

Merged
merged 11 commits into from Dec 8, 2020
Merged

clean_site default to preview = TRUE #1973

merged 11 commits into from Dec 8, 2020

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Dec 8, 2020

This will close #1971

  • preview = TRUE by default now - this will affect the behavior of Clean All button in website project Build pane
  • A message is output to console to inform about the file that could be removed.
  • clean_site(preview = FALSE) will effectively remove those files
  • Nothing is done if no file returned by the generator clean function
  • if quiet = FALSE and preview = FALSE, a message is also output to console - useful for knowing what happens in the Build pane

I also

  • refactored tests and add some for better coverage.
    • Added withr as Suggests because it is really useful for clean test helper.
  • adapted mark_dirs from bookdown - will do a PR in xfun

One question remaining:

  • Should clean_site() check for existence of the files and directories mentioned in message ?
  • Or should the generator$clean() function only return filepath that exists ?

I think the latter would be best but currently it does not - for example blogdown:

==> rmarkdown::clean_site()

These files and folders can probably be removed:

* blogdown
* public/
* content/_index.markdown
* content/post/2015-07-23-r-rmarkdown/index.html
* static/rmarkdown-libs

Use `rmarkdown::clean_site(preview = FALSE)` to remove them.

blogdown folder does not exist, static/rmarkdown-libs neither (because page bundle I think). I find it puzzling that it is output in the message

What do you think ?

@cderv cderv requested a review from yihui December 8, 2020 13:03
@cderv
Copy link
Collaborator Author

cderv commented Dec 8, 2020

Also I have opened #1974 for the idea we shared last Friday.

If you find it interesting and good enough in its implementation, we could ship it with this. Otherwise, we have time to think about it.

R/render_site.R Outdated Show resolved Hide resolved
#' @export
clean_site <- function(input = ".", preview = FALSE, quiet = FALSE,
clean_site <- function(input = ".", preview = TRUE, quiet = FALSE,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would it be interesting to have an option for preview to change the default behavior of the IDE button to FALSE ?

Not so sure, so I did not add it

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that could be useful (e.g., preview = getOption('rmarkdown.clean_site.preview', TRUE)), but let's wait until someone asks for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I remember this method of yours now - not used to that: get ideas but wait for people asking ! 😄

Copy link
Member

Choose a reason for hiding this comment

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

You can definitely also proactively ask if anyone wants this feature in advance. I just don't have the energy to do this myself :)

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

  • Should clean_site() check for existence of the files and directories mentioned in message ?
  • Or should the generator$clean() function only return filepath that exists ?

I'd prefer the latter. I'll fix the problem in blogdown. Thanks!

#' @export
clean_site <- function(input = ".", preview = FALSE, quiet = FALSE,
clean_site <- function(input = ".", preview = TRUE, quiet = FALSE,
Copy link
Member

Choose a reason for hiding this comment

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

Yes, that could be useful (e.g., preview = getOption('rmarkdown.clean_site.preview', TRUE)), but let's wait until someone asks for it.

R/render_site.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
R/render_site.R Outdated Show resolved Hide resolved
@yihui yihui merged commit f229a13 into master Dec 8, 2020
@yihui yihui deleted the clean-site branch December 8, 2020 18:50
yihui added a commit to rstudio/blogdown that referenced this pull request Dec 8, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clean_site should default to preview TRUE to prevent deletion without notice
2 participants