Greater flexibility in repo specification #1036

maelle · 2020-12-01T15:24:01Z

kulbhushanchand · 2021-05-23T04:36:59Z

Patiently waiting for the merger of this pull-request.

maelle · 2021-05-24T15:24:08Z

Maybe I should add tests first. 🤔

maelle · 2021-06-10T11:21:15Z

R/bs4_book.R

@@ -615,3 +664,43 @@ bs4_check_dots <- function(...) {
    )
  }
 }
+
+# test helpers -----------------------------------------------------------------


it's plural but there's only one for now :-)

is this only for test ? Meaning test helpers are stored inside the source of package ?
We usually store theme inside the test folder directly.

Also this function duplicates with bookdown::book_skeleton(), or the internal bookdown_skeleton() used for template. Maybe we can adapt those for the need of bs4_book(). It should be just a matter of rendering with special format (render_book(output_format = bookdown::bs4_book()))

So the function should actually

call bookdown::book_skeleton

modify files if needed (content, YAML configs)

call render_book?

modify files if needed (content, YAML configs)

Do you need to have special content in the Rmd for your test ? You should be able to control most of the config behavior from render_book()

cderv

I just add a quick look but here are a few comments.

Thanks!

R/bs4_book.R

cderv · 2021-06-10T12:51:07Z

R/bs4_book.R

@@ -615,3 +664,43 @@ bs4_check_dots <- function(...) {
    )
  }
 }
+
+# test helpers -----------------------------------------------------------------


is this only for test ? Meaning test helpers are stored inside the source of package ?
We usually store theme inside the test folder directly.

Also this function duplicates with bookdown::book_skeleton(), or the internal bookdown_skeleton() used for template. Maybe we can adapt those for the need of bs4_book(). It should be just a matter of rendering with special format (render_book(output_format = bookdown::bs4_book()))

tests/testthat/helper-bs4_book.R

maelle · 2021-06-14T10:59:33Z

tests/testthat/helper-bs4_book.R

+    name = name,
+    title = title,
+    author = author,
+    index_metadata = metadata,


in the end the current test doesn't use it but I feel it might be useful?

😅 it may not use it but as you set it to NULL in the test, you did not see that you created an issue in the first place as it has no default. 😉

I think we should stick to what is useful now - if we need it, it will be easy to add. If we don't, we'll have it in the codebase for nothing. Not great IMO.
It is also easier to adapt / modify when we don't have code part we try to preserve whereas they are not really used.

cderv

That looks great. I made a few comments.

About tests for this special format that works only on suggested package, do you know if CRAN expect tests to work only with Imports package or Suggests is ok ?

If they don't I believe we may need to add checks in tests or skip on CRAN all the testthat part so that is complies.

About xml2 used in testing for HTML output, is this something you have done in pkgdown also ?

Also, do we need to add something into the bs4_book() doc about github-repo field in index.Rmd YAML ? Do we need to set this metadata value to the same value as in this repo field ?

R/bs4_book.R

R/skeleton.R

tests/testthat/helper-bs4_book.R

cderv · 2021-06-15T09:51:58Z

tests/testthat/helper-bs4_book.R

+    name = name,
+    title = title,
+    author = author,
+    index_metadata = metadata,


😅 it may not use it but as you set it to NULL in the test, you did not see that you created an issue in the first place as it has no default. 😉

I think we should stick to what is useful now - if we need it, it will be easy to add. If we don't, we'll have it in the codebase for nothing. Not great IMO.
It is also easier to adapt / modify when we don't have code part we try to preserve whereas they are not really used.

tests/testthat/helper-bs4_book.R

maelle · 2021-06-15T12:38:58Z

That looks great. I made a few comments.

Thanks a lot for the feedback!

About tests for this special format that works only on suggested package, do you know if CRAN expect tests to work only with Imports package or Suggests is ok ?
If they don't I believe we may need to add checks in tests or skip on CRAN all the testthat part so that is complies.

Good point, I have added a helper skipper. Cf also r-lib/testthat#1398

About xml2 used in testing for HTML output, is this something you have done in pkgdown also ?

Definitely. Example 1, example 2.

Also, do we need to add something into the bs4_book() doc about github-repo field in index.Rmd YAML ? Do we need to set this metadata value to the same value as in this repo field ?

But bs4_book() does not use github-repo at all at the moment? Even in #1034 I haven't added. I actually do not know what this meta property does, actually. Maybe it should be tackled in that other PR?

to see the logic clearer imo.

…t_installed()

I think it works here because library(bookdown) is called and the function is found. But sometimes it does not, so I prefer to use the book namespaced name

cderv

That is ok. I'll take it from here.

[skip ci]

cderv · 2021-06-24T14:29:30Z

@yihui just so you know:

As discussed this includes new testthat infrastructure for bs4_boo() testing. Everything is skipped when either deps are not installed or Pandoc is not available. So tests should be run on CRAN if both condition are TRUE. This is as we dicussed I believe but we can skip on CRAN if it causes trouble somehow.

For the rest, it concerns the bs4_book() output only, excep a small tweak in bookdown::book_skeleton() to support a different path than working dir.

yihui · 2021-06-24T14:48:11Z

Sounds good. Thanks for letting me know!

maelle added 3 commits December 1, 2020 16:23

Greater flexibility in repo specification

8eee106

add detail

9f8109e

oops

3953519

cderv added the bs4_book 🥾 label Dec 1, 2020

maelle mentioned this pull request Feb 1, 2021

Make "edit this page" and "view source" link customizable in bs4_book #1070

Closed

3 tasks

maelle mentioned this pull request Mar 9, 2021

add github source url ropensci-books/http-testing#89

Closed

kulbhushanchand mentioned this pull request May 23, 2021

Update documentation theme to bs4_book kulbhushanchand/BioNES#4

Closed

apreshill added this to the v0.23 milestone May 29, 2021

maelle added 5 commits June 10, 2021 13:04

add test

ea41e94

silent

ab4a1bf

actually silent

3521f28

better test

39113ed

more testing

ab1ec43

maelle commented Jun 10, 2021

View reviewed changes

fix

a8fd9e9

cderv reviewed Jun 10, 2021

View reviewed changes

maelle added 5 commits June 14, 2021 12:17

tweak functions

2036c81

change icon

2d52f3f

move helper

3d54adf

tweak

90427b6

more flexibility

1ab5a08

maelle commented Jun 14, 2021

View reviewed changes

tests/testthat/helper-bs4_book.R Outdated Show resolved Hide resolved

maelle commented Jun 14, 2021

View reviewed changes

maelle added 2 commits June 14, 2021 13:00

Merge branch 'master' into repobs4

54b7195

Update DESCRIPTION

04da37f

cderv requested changes Jun 15, 2021

View reviewed changes

maelle added 2 commits June 15, 2021 14:22

helper

3e1303d

add skips

19f7af5

changes

2abbe5c

maelle added 2 commits June 15, 2021 14:39

oops

5674f4b

ouch

091fb49

maelle requested a review from cderv June 15, 2021 12:48

maelle added 2 commits June 15, 2021 14:51

rename

e4146d1

is this what @cderv had in mind?

d60be79

cderv self-assigned this Jun 24, 2021

cderv added 11 commits June 24, 2021 14:43

required for using local_tempdir()

f0aeb81

re-document

f144bc9

adapt style for this file

0ffcbd9

small tweak

81f4940

to see the logic clearer imo.

Add comment on bs4_book_deps

db1b6f1

tweak book_skeleton

66fe6e5

prefer vapply for type stability and return invisibly like skip_if_no…

0904763

…t_installed()

output_format should be the all function.

666de2c

I think it works here because library(bookdown) is called and the function is found. But sometimes it does not, so I prefer to use the book namespaced name

Add link to issue

b758da0

Add pandoc testthat helpers from rmarkdown test suite

1e190f4

be sure to not run the test is there is no Pandoc available

1b83cbe

cderv approved these changes Jun 24, 2021

View reviewed changes

cderv and others added 3 commits June 24, 2021 15:43

Add a NEWS bullet

7e8351b

Merge branch 'master' into repobs4

b0534b4

bump version

8ae6715

[skip ci]

cderv merged commit dfa56b5 into rstudio:master Jun 24, 2021

cderv removed their assignment Jun 24, 2021

apreshill mentioned this pull request Aug 6, 2021

The "edit" feature in bookdown::bs4 navbar assumes a branch named "master" #1220

Closed

cderv mentioned this pull request Sep 20, 2021

"View Book Source" link always points to the "base" dir instead of "base + subdir" #1259

Open

github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2021

Greater flexibility in repo specification #1036

Greater flexibility in repo specification #1036

Uh oh!

Conversation

maelle commented Dec 1, 2020

Uh oh!

kulbhushanchand commented May 23, 2021

Uh oh!

maelle commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cderv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cderv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maelle commented Jun 15, 2021

Uh oh!

cderv left a comment

Choose a reason for hiding this comment

Uh oh!

cderv commented Jun 24, 2021

Uh oh!

yihui commented Jun 24, 2021

Uh oh!

Uh oh!