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

Polish code and docs re: options for DESCRIPTION fields #367

Merged
merged 15 commits into from May 29, 2018
Merged

Polish code and docs re: options for DESCRIPTION fields #367

merged 15 commits into from May 29, 2018

Conversation

@jennybc
Copy link
Member

@jennybc jennybc commented May 26, 2018

Closes #233 create_package() should document options

Closes #159 Review all uses of getOption("devtools.SOMETHING")

Rationalize and document the use of options and internal defaults re: DESCRIPTION fields. Key principles:

  • Prefer usethis options to devtools options, but consult devtools options for backwards-compatibility
  • Eliminate redundant consultation of options
  • Adopt a modifyList() mentality vs. the "all or nothing" mentality of %||%
  • Add tests to make sure the order of precedence is what we say it is
@jennybc jennybc changed the title WIP re: options Polish code and docs re: options for DESCRIPTION fields May 28, 2018
@jennybc jennybc requested review from hadley and jimhester May 28, 2018
@@ -48,22 +58,15 @@ build_description <- function(name, fields = list()) {
}

build_description_list <- function(name, fields = list()) {
author <- getOption("devtools.desc.author") %||%

This comment has been minimized.

@hadley

hadley May 28, 2018
Member

We're abandoning these individual options in favour of the omnibus usethis.description? If yes, need to call out in the NEWS (although it probably doesn't affect many people)

This comment has been minimized.

@jennybc

jennybc May 28, 2018
Author Member

Oh, I failed to see that this wasn't just grabbing the "author" element of the "devtools.desc" list.

But it does seem like something we could probably drop. Or switch entirely to "devtools.desc.XYZ". Supporting usethis and devtools options, in individual and ombibus form seems unnecessary.

This comment has been minimized.

@hadley

hadley May 28, 2018
Member

Yeah, I'm fine with dropping it.

getOption("devtools.desc") %||%
list()
## the definitive source of user-supplied info: in this call or via options
fields <- utils::modifyList(

This comment has been minimized.

@hadley

hadley May 28, 2018
Member

It might be worth doing some light type checking here - e.g. that it's a list with names?

This comment has been minimized.

@hadley

hadley May 28, 2018
Member

And should all this logic be moved into build_description()? I think that would make the tests simpler since you wouldn't need so many temporary projects

This comment has been minimized.

@jennybc

jennybc May 28, 2018
Author Member

I'll try to move things around.

Part of the reason I tested via use_description(), though, was to test that directly specified fields override options & defaults, but in a targetted manner.

This comment has been minimized.

@hadley

hadley May 28, 2018
Member

Yeah, I think it would be better to move all that code out so you can test independently of the writing.

@jennybc
Copy link
Member Author

@jennybc jennybc commented May 29, 2018

I think this does everything we discussed @hadley:

  • Drop support of individual devtools' options and document this in NEWS.
  • Add check that user's fields is a dictionary-ish list.
  • Consolidate
    • data: option- and package-based defaults exposed via use_description_defaults()
    • logic: user's fields > usethis option > devtools option > usethis defaults, enacted in build_description_list()
  • Tests now focus on build_description_list()
#' }
use_description <- function(fields = NULL) {
name <- project_name()
check_package_name(name)
if (!is.null(fields)) check_is_named_list(fields)

This comment has been minimized.

@hadley

hadley May 29, 2018
Member

If you moved this one line lower you could drop the conditional.

This comment has been minimized.

@jennybc

jennybc May 29, 2018
Author Member

done

ByteCompile = "true"
build_description_list <- function(fields = list()) {
defaults <- use_description_defaults()
defaults <- utils::modifyList(

This comment has been minimized.

@hadley

hadley May 29, 2018
Member

It feels to me like this belongs in use_description_defaults()

This comment has been minimized.

@jennybc

jennybc May 29, 2018
Author Member

I actually made use_description_defaults() "data only" on purpose. It's the only easy way to see the usethis field defaults w/o reading source. Unless you care deeply, will leave this.

context("use_description")

test_that("build_description_list() defaults to values built into usethis", {
withr::with_options(

This comment has been minimized.

@hadley

hadley May 29, 2018
Member

I think it's easier to read if you use local_options()

This comment has been minimized.

@jennybc

jennybc May 29, 2018
Author Member

AHA! done.

fields = list(Title = "aaa", URL = "https://www.r-project.org")
)
## from user's fields
expect_identical(d$Title, "aaa")

This comment has been minimized.

@hadley

hadley May 29, 2018
Member

Why test two fields from each case? Isn't one enough?

This comment has been minimized.

@jennybc

jennybc May 29, 2018
Author Member

One is an overwrite, one is novel. But I did remove one test case re: default values.

@hadley
hadley approved these changes May 29, 2018
@jennybc jennybc merged commit 5fca4ce into master May 29, 2018
6 checks passed
6 checks passed
codecov/patch 100% of diff hit (target 62.91%)
Details
codecov/project 63.13% (+0.22%) compared to f9becb7
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jennybc jennybc deleted the options branch May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants