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

function for making new blog posts #171

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

topepo
Copy link
Collaborator

@topepo topepo commented Apr 16, 2024

For #22

Adds some dependencies (fs, whoami).

yml_values <- yml_values[names(yml_values) != "categories"]
}
yml_values <- yaml::as.yaml(yml_values)
yml_values <- paste0("---\n", yml_values, "---\n")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, maybe there is a better API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While dealing with YAML in quarto context we have the issue of yaml R package being 1.1 spec, and Quarto using 1.2 spec.

So it requires some specific handlers as workaround. See

quarto-r/R/utils.R

Lines 6 to 16 in d71b135

write_yaml <- function(x, file) {
handlers <- list(
# Handle yes/no from 1.1 to 1.2
# https://github.com/vubiostat/r-yaml/issues/131
logical = function(x) {
value <- ifelse(x, "true", "false")
structure(value, class = "verbatim")
}
)
yaml::write_yaml(x, file, handlers = handlers)
}

We probably need to adapt for generating not in a file, or refactor the handlers so that we can use them in other functions.

Regaring ---, I believe this is good enough.

This post creation may also be done through templating 🤔 Having a whisker template (or else) to file out with some information. Probably more complex than necessary, but we did that for bookdown skeleton for example using placeholder in template file (https://github.com/rstudio/bookdown/blob/f244cf12bf2c2d7106ac6322b2b2a5796d4ef0c8/R/skeleton.R#L66-L83)

I am fine with current way

@topepo topepo marked this pull request as ready for review April 16, 2024 18:34
@topepo topepo requested a review from cderv April 18, 2024 16:28
@topepo
Copy link
Collaborator Author

topepo commented Apr 18, 2024

GHA failures are now down to infrastructure issues, not failing R CMD check

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

Thanks a lot ! This will be really useful.

Do you think this will be used more interactively or not ? Just curious about your use case.

Just a few suggestions below. I see that we may need to really start using fs

Also, I am wondering if we should have more check that this function is used in the right place.

  • website project where blog is supported not another type or no project
  • called in the root of the project so that posts folder is created in the right place
  • or no matter where called, the new post is created in the right place from the root project

Again, thanks !!

DESCRIPTION Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

As those two deps are only required for the new blog post function, and this function is aimed to be used mostly interactively, I wonder if we could rely on rlang::check_installed()

By using if(rlang::check_installed("whoami")), user will be asked to install if missing when used interactively.

This would allow to make them suggested dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For fs, we probably could start using it in this package.

@@ -1,5 +1,7 @@
# quarto (development version)

- Added a `new_blog_post()` function (#22).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Added a `new_blog_post()` function (#22).
- Added a `new_blog_post()` function (thanks, @topepo , #22).

R/new-blog-post.R Show resolved Hide resolved
R/new-blog-post.R Show resolved Hide resolved
#' default, the title is adapted as the directory name.
#' @param open A logical: have the default editor open a window to edit the
#' `index.qmd` file?
#' @param call A call object for reporting errors.
Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding is that the prefered way is to use

⁠@inheritParams rlang::args_error_context

From https://rlang.r-lib.org/reference/args_error_context.html and usage at several tidyverse packages

yml_values <- yml_values[names(yml_values) != "categories"]
}
yml_values <- yaml::as.yaml(yml_values)
yml_values <- paste0("---\n", yml_values, "---\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

While dealing with YAML in quarto context we have the issue of yaml R package being 1.1 spec, and Quarto using 1.2 spec.

So it requires some specific handlers as workaround. See

quarto-r/R/utils.R

Lines 6 to 16 in d71b135

write_yaml <- function(x, file) {
handlers <- list(
# Handle yes/no from 1.1 to 1.2
# https://github.com/vubiostat/r-yaml/issues/131
logical = function(x) {
value <- ifelse(x, "true", "false")
structure(value, class = "verbatim")
}
)
yaml::write_yaml(x, file, handlers = handlers)
}

We probably need to adapt for generating not in a file, or refactor the handlers so that we can use them in other functions.

Regaring ---, I believe this is good enough.

This post creation may also be done through templating 🤔 Having a whisker template (or else) to file out with some information. Probably more complex than necessary, but we did that for bookdown skeleton for example using placeholder in template file (https://github.com/rstudio/bookdown/blob/f244cf12bf2c2d7106ac6322b2b2a5796d4ef0c8/R/skeleton.R#L66-L83)

I am fine with current way

make_post_yaml <- function(title, ...) {
default_values <- list(
title = tools::toTitleCase(title),
author = tools::toTitleCase(whoami::fullname("Your name")),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could document for .Rd file that whoami is used as default when available

We could also easily make this deps a suggest by using it only if available

tools::toTitleCase(if(rlang::is_installed("whoami")) whoami::fullname("Your name") else "Your name")

Comment on lines +6 to +9
temp_dir <- withr::local_tempdir()
dir_path <- fs::path(temp_dir, "test-blog-project")

withr::defer(fs::dir_delete(dir_path), envir = rlang::current_env())
Copy link
Collaborator

Choose a reason for hiding this comment

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

withr::local_tempdir() already include a defer call to unlink() recursively its content. So I believe no need for a new defer call

Comment on lines +14 to +15
setwd(dir_path)
withr::defer(setwd(current_dir), envir = rlang::current_env())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually I use withr::local_dir() to change working directory for the rest of the test

This should be enough

withr::local_dir(fs::path(temp_dir, "test-blog-project"))

setwd(dir_path)
withr::defer(setwd(current_dir), envir = rlang::current_env())

Sys.setenv(FULLNAME="Max Kuhn")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should keep using withr for that

Suggested change
Sys.setenv(FULLNAME="Max Kuhn")
withr::local_envvar(list(FULLNAME = "Max Kuhn"))

@cderv
Copy link
Collaborator

cderv commented May 15, 2024

GHA failures are now down to infrastructure issues, not failing R CMD check

I have updated main to solve this. It should be ok now.

@cderv cderv linked an issue Sep 24, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: new_post function / RStudio Addin
2 participants