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

Use tutorial, round 2 #645

Merged
merged 18 commits into from
Mar 15, 2019
Merged

Use tutorial, round 2 #645

merged 18 commits into from
Mar 15, 2019

Conversation

angela-li
Copy link
Contributor

@angela-li angela-li commented Mar 4, 2019

Closes #314 use_tutorial()

Making this PR properly from a branch instead of master of my forked repo, incorporating code review edits in #584

Merge me instead of #584! 😄

@angela-li
Copy link
Contributor Author

Looks like builds are failing because learnr isn't installed in Appveyor/Travis?

Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

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

Thanks! I made a few comments.

DESCRIPTION Outdated Show resolved Hide resolved
R/tutorial.R Outdated Show resolved Hide resolved
R/tutorial.R Outdated Show resolved Hide resolved
tests/testthat/test-use-tutorial.R Show resolved Hide resolved
tests/testthat/test-use-tutorial.R Outdated Show resolved Hide resolved
@jennybc
Copy link
Member

jennybc commented Mar 4, 2019

There's a test failing on AppVeyor and Travis. You probably need to use the same mocking strategy we use elsewhere to get around the need for learnr to be installed.

with_mock(
## need to pass the check re: whether roxygen2md is installed
`usethis:::check_installed` = function(pkg) TRUE, {
scoped_temporary_package()
cat(
"RoxygenNote: 6.0.1\n",
file = proj_path("DESCRIPTION"),
append = TRUE
)
expect_error_free(use_roxygen_md())
}
)

@jennybc
Copy link
Member

jennybc commented Mar 4, 2019

@garrettgman If you took this for a spin, that would be great. It basically looks good to me, so I plan to merge soon, after we wrap up the loose ends.

@jennybc
Copy link
Member

jennybc commented Mar 4, 2019

Can you also please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

@jennybc
Copy link
Member

jennybc commented Mar 15, 2019

Good grief it took me a while to get the test and mocking sorted -- probably just as well that I was the one to suffer through that.

Thanks!

@jennybc jennybc merged commit 8fa45c0 into r-lib:master Mar 15, 2019
jennybc pushed a commit that referenced this pull request Mar 28, 2019
* Add use_rcpp_armadillo() and use_rcpp_eigen(). (closes #421).

Note: This is the second coming of PR #601 based on feedback from @jennybc. She requested the design pattern in #645 for testing a package without needing to add it to the Suggests field. This is important as RcppArmadillo has a hard dependency on R >= 3.2.

* usethis itself doesn't need RcppEigen in Suggests

* Style
@angela-li
Copy link
Contributor Author

You caught me at a busy time, but thanks for helping out with the rest of the edits! Appreciate your support on this (thanks for figuring out the mocking). It's exciting to see use_tutorial added into usethis 👏

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