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

Add handout options #527

Merged
merged 20 commits into from
Oct 16, 2023
Merged

Add handout options #527

merged 20 commits into from
Oct 16, 2023

Conversation

froggleston
Copy link
Contributor

This PR adds a new option in the config.yaml to support configuration of whether a handout should be included.

This removes the need to supply sandpaper.handout = TRUE in the options() function, but keeps the functionality to either specify handout: true or handout: "/path/to/handout_file.R"

R/build_markdown.R Outdated Show resolved Hide resolved
@zkamvar
Copy link
Contributor

zkamvar commented Oct 3, 2023

I have no clue why the checks are not starting. @froggleston, could you merge in the main branch, add NEWS, and bump the version number to devel as described in https://carpentries.github.io/workbench-dev/releases.html?

@zkamvar
Copy link
Contributor

zkamvar commented Oct 9, 2023

I've added you to the DESCRIPTION and also fixed some tests that were breaking because of things.

At the moment Windows is failing for weird reasons:

  ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure ('test-build_markdown.R:173:3'): Artifacts are accounted for ────────
  fs::path_file(a) (`actual`) and c(figs, b, "code-handout.R") (`expected`) don't have the same values.
  * Only in `actual`: "code-handout.R"
  
  
  [ FAIL 1 | WARN 0 | SKIP 24 | PASS 1005 ]
  Error: Error: Test failures
'D:/a/_temp/RtmpysTSvn/working_dir/RtmpEVreqL/file15c4ce49f6/lesson-example' successfully removed
  Error in vapply(X, FUN, FUN.VALUE = character(1), ..., USE.NAMES = USE.NAMES) : 
    values must be type 'character',
   but FUN(X[[1]]) result is type 'logical'
  Calls: test_check ... test_dir -> test_files -> test_files_serial -> <Anonymous>
  Execution halted

There are two things to notice:

  1. the test failure is not actually a failure because it contains the value "code-handout.R"
  2. There is a weird error coming from testthat itself. It will be worthwhile running the test-build_markdown.R test file in a windows environment (👋🏼 @klbarnes20) and see if we can reproduce the error.

zkamvar added 13 commits October 8, 2023 17:18
This is one of the weird litte things that I need to get better at
controlling. When we test these things, the global metdata is affected,
but it's not possible (at the moment) to _remove_ metadata without
resetting everything, so instead, we need to set the global metadata
option for "handout" to NULL.

In the words of the author of a C++ library that I rewrote in R for my
Ph. D.:

> This is a bloody awful nasty hack ... and will give us grief
> elsewhere. Find a better way to do this
@zkamvar
Copy link
Contributor

zkamvar commented Oct 16, 2023

I have finally figured out why the windows tests have been failing: there is a weird bug in gert where it's apparently having trouble removing a bare repository (that we use as the remote) because of a pack file with incorrect permissions:

  ℹ removing 'sandpaper-local' ('D:/a/_temp/RtmpuIca1V/working_dir/RtmpMr9uia/REMOTE-1a9826d69e4')
  ✖ Error trying to remove remote
  [EPERM] Failed to remove
  'D:/a/_temp/RtmpuIca1V/working_dir/RtmpMr9uia/REMOTE-1a9826d69e4/objects/pack/pack-07442ae371f99d1efb5939a0db60b2f13f1147c5.idx':
  operation not permitted

In any case, this is not going to be a big problem because this is explicitly in the context of tests and not in production code (i.e. we do not delete our remote in production).

@zkamvar
Copy link
Contributor

zkamvar commented Oct 16, 2023

Thank you @froggleston and @klbarnes20!

@zkamvar zkamvar merged commit fbf1fa2 into main Oct 16, 2023
@zkamvar zkamvar deleted the add-handout-options branch October 16, 2023 16:36
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.

2 participants