-
Notifications
You must be signed in to change notification settings - Fork 25
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
implemented document_date field #85
Conversation
Started from scratch re eddelbuettel#63
For now I resorted to deleting the repo and forking it again |
I just had a very first quick glance at it and it looks good. One minor suggestion for the manual page which I type up later. For an example run, any odd document plus an explicit YAML header argument (As for changing names of existing options: not great. Published "APIs" for users and all that....) |
@@ -35,21 +35,32 @@ | |||
#' suffix can be omitted; defaut is no bibliography.} | |||
#' \item{\code{doi}}{(Optional but recommended) A free-format entry | |||
#' suitable for a doi or url referencing the document or its | |||
#' underlying code.} \item{\code{fontsize}}{(Optional) Document | |||
#' underlying code.} | |||
#' \item{\code{fontsize}}{(Optional) Document |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the missing line break.
R/pinp.R
Outdated
#' fontsize, default is 9pt.} | ||
#' \item{\code{footer_contents}}{(Optional) A free-format entry for | ||
#' text placed in the footer, useful to associate with a package or | ||
#' volume, default is \sQuote{Package Vignette}.} | ||
#' \item{\code{date_subtitle}}{(Optional) An _optional_ free-form text string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This irked me a little when I first glanced at his earlier. First off, it repeats 'optional' twice within three words. Second, we are old school here and do NOT use markdown.
So I just fixed this and will commit back into your repo and the PR in a moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally good modulo the minor tweak I'll commit in a second.
R/pinp.R
Outdated
#' Could be used, for example, to mention the bibliographic info in a | ||
#' post-print. If not specified, defaults to "This version was compiled on | ||
#' {current date}"} | ||
#' \item{\code{document_date}}{(Optional) An _optional_ free-form text string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment.
I will take silence from @coatless as absence of concerns and merge this :) Thanks for the new feature, being able to set both optionally is nice. |
@eddelbuettel sorry, saw the earlier PR that was closed out. Missed this. That said, this looks good! Thanks @ikashnitsky! |
Started from scratch
in response to the recent discussion in #63
Discussion part
Maybe there should be a bit more consistency between
date_subtitle
anddocument_date
. In my view their behaviour can be a bit confusing since both default to\today
. I'd suggest to renamedate_subtitle
intosubtitle
orinfoline
– just drop date from default in this line. The date of the document can stay only in the footer – freely specified as text with the newdocument_date
YAML param or resolving to\today
by default. What do you think?