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

Roxygen 7 #2524

Merged
merged 18 commits into from Jan 28, 2020
Merged

Roxygen 7 #2524

merged 18 commits into from Jan 28, 2020

Conversation

infotroph
Copy link
Member

@infotroph infotroph commented Jan 28, 2020

Description

First commit in this PR is just bumping Roxygen and recompiling docs:

git pull upstream develop
git switch -c roxygen-7
Rscript -e 'install.packages("roxygen2")'
make clean
make document
  • **/DESCRIPTION: version bump in the RoxygenNote field, no other changes
  • **/NAMESPACE: only modules/emulator changes, by now exporting S4 classes jump and mvjump. I see no problem with this, but someone who understands S4 classes better than me should double-check.
  • */man*.Rd: Changes are mostly minor and obviously correct: line-wrapping, tweaks to translation between foo and \code{foo} vs \verb{foo}, better cross-package reference links, etc.Exceptions to the "obviously correct" rule are handled below.

I reviewed the above diff by hand, took notes on changes that looked wrong, then committed it and moved on to phase two. The remaining commits are me finding and resolving issues uncovered by the version change:

  • Removed backslash escaping from percent signs inside Markdown text, because Roxygen now provides the escaping automatically. But beware: percents still need to be escaped in the packages that don't use Markdown (which is still most of them).
  • Apparently the recommended way of providing whole-package documentation has changed slightly; I updated PEcAn.DB, the only one where it changed the result.
  • ACTION NEEDED: When documenting multiple functions in the same Rd file, Roxygen used to include multiple copies of any function arguments that were specified multiple times. It now includes only one copy and ignores the others, even if they're worded differently. There were a few places it wasn't obvious how to resolve this; @ashiklom and @para2x, please see my comments below.
  • Turns out PEcAn.data.atmosphere::met2CF.PalEONregional was defined twice, copy-paste identically, in the same file. Fixed that.
  • Various small formatting / example / Roxygen syntax issues that weren't causing check errors before but started now. All needed fixing before too, the update just made them louder.

Motivation and Context

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.


\item{Q}{Process covariance matrix given if there is no data to estimate it.}

\item{restart}{Used for iterative updating previous forecasts. When the restart is TRUE it read the object in SDA folder written from previous SDA.}
Copy link
Member Author

@infotroph infotroph Jan 28, 2020

Choose a reason for hiding this comment

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

This file used to contain duplicate definitions for the parameters that are shared between sda.enkf and sda.enkf.original, but when rebuilt using Roxygen 7 it now only lists each parameter name once.

That seems desirable for functions documented in the same file, but note that where definitions differ only the one from sda.enkf is used (and the definitions from sda.enkf.original are not visible anywhere). @para2x please check carefully (the whole diff of this file, not just these lines I'm commenting at) and advise: Is that the correct result here?

Copy link
Member

@ashiklom ashiklom left a comment

Choose a reason for hiding this comment

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

I don't see any problems. Thanks for doing it! One less repository for which I need to hold back R package updates...

@mdietze mdietze merged commit 730cbc6 into PecanProject:develop Jan 28, 2020
Copy link
Member

@robkooper robkooper left a comment

Choose a reason for hiding this comment

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

only comment is the % that is escaped for latex I believe using %. Not sure if the pdf generation is smart enough for that.

@@ -23,7 +23,7 @@
##' @param eps used to set artificial bound on min width / max height of bins as described in Denby and Mallows (2009) on page 24.
##' @param xlab is label for the x axis
##' @param plot = TRUE produces the plot, FALSE returns the heights, breaks and counts
##' @param lab.spikes = TRUE labels the \% of data in the spikes
##' @param lab.spikes = TRUE labels the % of data in the spikes
Copy link
Member

Choose a reason for hiding this comment

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

does this still work as expected with the pdf version. % is a latex char and I think that is why it was escaped.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to:
Screen Shot 2020-01-28 at 6 23 14 PM

My understanding is that the PDF version is built from the Rd files without re-invoking Roxygen, and it's still spelled \% in the Rd file. this is just a switch from us typing it to Roxygen inserting it.

@infotroph infotroph deleted the roxygen-7 branch January 29, 2020 10:49
@robkooper robkooper added this to the 1.8.0 milestone Apr 14, 2020
@istfer istfer mentioned this pull request Aug 20, 2020
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.

None yet

4 participants