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

Combine section_init() and create_subdir() #2646

Merged
merged 6 commits into from
Jun 7, 2024
Merged

Conversation

hadley
Copy link
Member

@hadley hadley commented Jun 7, 2024

  • We can figure out depth based on whether or not a subdir is supplied
  • Ensures all user facing functions have the same interface (i.e. have override)
  • Brings together directory checking code in one place

Fixes #2644

@olivroy I started reviewing #2645 but then I wondered if maybe we could simplify things by unifying section_init() and create_subdir() and that resulted in this PR 😄

* We can figure out `depth` based on whether or not a `subdir` is supplied
* Ensures all user facing functions have the same interface
* Brings together directory checking code in one place

Fixes #2644
@hadley hadley requested a review from olivroy June 7, 2024 12:48
subdir = NULL,
override = list(),
.frame = parent.frame()) {
rstudio_save_all()
Copy link
Member Author

Choose a reason for hiding this comment

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

We want to save all files before we read in the yaml.

Copy link

github-actions bot commented Jun 7, 2024

@olivroy
Copy link
Collaborator

olivroy commented Jun 7, 2024

I think it is a good idea. But this somehow seems to break the workflow I created in #2571.

i.e. to call build_home() yields the error message that you need to call init_site().

@hadley think I found the reason. I think that if 'check_dest_is_pkgdown()finds no file or dir inside docs/dev, it should call init site.unlink("docs/", recursive = TRUE)` sometimes leave behind an empty folder.

@olivroy
Copy link
Collaborator

olivroy commented Jun 7, 2024

Is the no-pandoc failure stochastic ? If so, maybe we should skip this test if no pandoc?

@hadley
Copy link
Member Author

hadley commented Jun 7, 2024

I figured out a reprex:

src_path <- withr::local_tempdir()
writeLines(path(src_path, "README.md"), "Hi")
description <- desc::desc("!new")
description$set("Package", "testpackage")
description$set("Title", "A test package")
description$write(file = path(src_path, "DESCRIPTION"))    

build_home(src_path)
clean_site(src_path)
build_home(src_path)

@hadley
Copy link
Member Author

hadley commented Jun 7, 2024

Ah yeah, the problem is we should check for files, not whether or not the directory exists.

Copy link
Collaborator

@olivroy olivroy left a comment

Choose a reason for hiding this comment

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

Now works for me, thanks! I like the idea. Will finish up #2642 after this. Maybe add that this closes #2645

Is this news worthy? probably not :)

Hi Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops

@hadley hadley merged commit 8defd98 into main Jun 7, 2024
13 checks passed
@hadley hadley deleted the supercharge-section-init branch June 7, 2024 15:53
@hadley
Copy link
Member Author

hadley commented Jun 7, 2024

IMO this isn't worth its own a news bullet because it's more of a refinement of existing work that already has news bullets.

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.

init_site() doesn't have an override argument
2 participants