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

Default title and description for package docs #392

Merged
merged 11 commits into from
Oct 10, 2015
Merged

Default title and description for package docs #392

merged 11 commits into from
Oct 10, 2015

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Oct 7, 2015

Using _PACKAGE as sentinel value (to avoid unlikely collision with an object actually named "PACKAGE").

No functionality yet, just to make sure we're on the same page here.

Fixes #349.

@@ -113,4 +113,6 @@ obj_type.refMethodDef <- function(x) "rcmethod"
#' @export
obj_type.function <- function(x) "function"
#' @export
obj_type.package_doc <- function(x) "package_doc"
Copy link
Member

Choose a reason for hiding this comment

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

I'd just call it "package" (it's implicitly about documentation)

Kirill Müller added 6 commits October 8, 2015 11:23
- instantiated when documenting a value _PACKAGE
- found a slightly hackish way to pass the base path down to object_defaults
- using roc_proc_text and mocking
if (identical(name, "_PACKAGE")) {
return(structure(
list(pkg_name = packageName(env),
base_path = getNamespaceInfo(ns, "path")),
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if there's a better way to find the base_path here (or in object_defaults.package()).

Copy link
Member

Choose a reason for hiding this comment

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

In object_defaults you might be able to do some manipulation of x$srcref$filename, but I don't know if that contains the full path

Copy link
Member Author

Choose a reason for hiding this comment

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

In object_defaults(), x doesn't have srcref -- only alias, value, and methods. Only the caller, add_defaults(), has access to srcref.

Copy link
Member

Choose a reason for hiding this comment

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

Oh true, but we could pass that on

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course: dae13b2. I'm not sure if this is really better, though. Please let me know which you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

I do prefer it, just because it keeps the division of responsibilities more clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, but with modified division of responsibilities: 0a6f850

@krlmlr krlmlr changed the title WIP: Default title and description for package docs Default title and description for package docs Oct 8, 2015
@krlmlr
Copy link
Member Author

krlmlr commented Oct 8, 2015

Works for me. More fields can be added later, such as author, version, ...

docType = "package",
title = as.character(desc$Title),
description = as.character(desc$Description),
aliases = c("NULL", paste0(pkg_name, "-package")), # See #202
Copy link
Member

Choose a reason for hiding this comment

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

Could you make the comment a bit more direct? i.e. "NULL" prevents addition of default aliases

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Kirill Müller added 4 commits October 8, 2015 21:48
- find_data() returns object that is sufficient to generate object defaults
    - in the case of a package, this is the DESCRIPTION
- object_defaults() only converts the defaults to tags
hadley added a commit that referenced this pull request Oct 10, 2015
Default title and description for package docs
@hadley hadley merged commit e12b332 into r-lib:master Oct 10, 2015
@hadley
Copy link
Member

hadley commented Oct 10, 2015

Thanks!

@jimhester
Copy link
Member

Maybe this is too close to the release to change, I think the underscore prefix for "_PACKAGE" is unnecessarily conservative. How often would someone previously be documenting a string of "PACKAGE". I could see someone documenting a PACKAGE object, but never a bare string. Can't we get largely the same collision guarantees by checking the "PACKAGE" object is a character(1) in the package environment (At https://github.com/klutometis/roxygen/pull/392/files#diff-00c18366780143a8dfe7c9420de32495R27 perhaps?)

Necessitating the leading underscore just complicates the typical case without a real benefit IMHO.

@hadley
Copy link
Member

hadley commented Oct 22, 2015

I like the underscore because it makes it clear that this is a special string. We might add others in the future.

@jimhester
Copy link
Member

Fair enough, consider me overruled 😄

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.

3 participants