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

added with_device and device_dev fns #37

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@richierocks
Contributor

richierocks commented Dec 6, 2015

For issue #29, now including the missing with_png.

richierocks added some commits Dec 6, 2015

@codecov-io

This comment has been minimized.

codecov-io commented Dec 6, 2015

Current coverage is 100.00%

Merging #37 into master will not affect coverage as of 18471aa

@@            master     #37   diff @@
======================================
  Files            8       8       
  Stmts          100     100       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit            100     100       
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of 18471aa

Powered by Codecov. Updated on successful CI builds.

NAMESPACE Outdated
@@ -1,9 +1,13 @@
# Generated by roxygen2: do not edit by hand
# Generated by roxygen2 (4.1.1): do not edit by hand

This comment has been minimized.

@krlmlr

krlmlr Dec 6, 2015

Member

Could you please update your roxygen2 installation?

R/devices.R Outdated
#' @rdname with_bmp
#' @export
with_postscript <- with_(postscript_dev, dev.off)

This comment has been minimized.

@krlmlr

krlmlr Dec 6, 2015

Member

The names "with_cairo_ps()" and "with_postscript()" look inconsistent to me.

R/devices.R Outdated
#' Temporarily use a graphics device.
#'
#' @template with
#' @param new \code{[named character]}\cr New graphics device

This comment has been minimized.

@krlmlr

krlmlr Dec 6, 2015

Member

Please double-check documentation of this argument.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Dec 6, 2015

Thanks, Richard. I've only skimmed through your patch. R CMD check seems to have complaints, see failing Travis build. Jim: Do you have further comments?

@jimhester

This comment has been minimized.

Member

jimhester commented Dec 7, 2015

We are going to need some tests written for these functions.

antialias <- match.arg(antialias) for jpeg_dev() and png_dev(), why is this necessary?

Note the formals differ for grDevices::png() for windows (https://github.com/wch/r-source/blob/trunk/src/library/grDevices/R/windows/png.R#L42) and unix (https://github.com/wch/r-source/blob/trunk/src/library/grDevices/R/unix/png.R#L40), which is also going to pose a problem with inconsistent documentation for us. grDevices has man/unix and man/windows folders, it is possible for external packages to do the same thing?

Also regarding documentation I would suggest you use a layout like

#' Graphics devices
#'
#' Temporarily use a graphics device.
#'
#' @rdname devices'
#' @template with
#' @param new \code{[named character]}\cr New graphics device
#' @seealso \code{\link[grDevices]{Devices}}
#' @examples
#' # dimensions are in inches
#' with_pdf(
#'   c(file = file.path(tempdir(), "test.pdf"), width = 7, height = 5), 
#'   plot(runif(5))
#' )
#' 
#' # dimensions are in pixels
#' with_png(
#'   c(file = file.path(tempdir(), "test.png"), width = 800, height = 600), 
#'   plot(runif(5))
#' )
NULL

#' @describeIn devices BMP device
#' @inheritParams grDevices::bmp
#' @export
with_bmp <- with_(bmp_dev, grDevices::dev.off)

#' @describeIn devices Cairo PDF device
#' @inheritParams grDevices::cairo_pdf
#' @export
with_cairo_pdf <- with_(cairo_pdf_dev, dev.off)

This will ensure all wrapped function arguments are correctly represented in the man page.

Also we have been avoiding @importFrom declarations in favor of explicitly using the fully qualified name pkg::name for all calls, could you please convert to that convention.

Thank you for the pull request, we are going to have to be careful with this because of the windows/unix differences. Luckily we have CIs for both, so just need to make sure R CMD check is clean for both cases.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Dec 7, 2015

In reply to #29 (comment) (I think implementation details are better discussed with the pull request that contains them):

Thanks. As you noted, the different function interfaces for png() are suboptimal. I've noted similar problems when implementing with_sink(), and ended up defining a slightly different interface. Would this also be possible for png()?

I think @inheritParams is nice if it works, but if it doesn't it might be possible to copy-paste the argument description and use it in a simple @param statement.

@jimhester

This comment has been minimized.

Member

jimhester commented Dec 7, 2015

R does support using separate windows/ and unix/ folders for both code and documentation out of the box.

The R and man subdirectories may contain OS-specific subdirectories named unix or windows. [1]

Also extends to the Collation field.

An OS-specific collation field (‘Collate.unix’ or ‘Collate.windows’) will be used in preference to ‘Collate’. [2]

However neither roxygen nor devtools support these alternate Collate directives or look for source files in the unix or windows. So we need to either

  1. Write / Tweak the documentation by hand
  2. Add a script which will fix the documentation/collation after running roxygen
  3. Add support for unix and windows subdirectories and Collation fields in roxygen and devtools.
  4. and 2. are both suboptimal solutions IMHO, so I would ideally want to do 3.
  • What error are you seeing that requires antialias <- match.arg(antialias)? Using NULL there works for both windows and linux for me?

An example branch using the OS specific directories is at https://github.com/jimhester/withr/tree/os_specific

@jimhester

This comment has been minimized.

Member

jimhester commented Dec 7, 2015

Nevermind, I guess https://ci.appveyor.com/project/jimhester/withr/build/1.0.110#L322 is the error.

Working example now at 00b26d3

@jimhester

This comment has been minimized.

Member

jimhester commented Sep 1, 2017

I merged this manually aee7d37 with additional updates, so we now have with_ and local_ variants for all of these devices. For the devices with system dependant interfaces (bmp, png, jpeg, tiff) the with_*() functions simply pass ... on rather than trying to expose the full interface.

@richierocks Thank you for your patience, sorry it took so long to get this merged!

@jimhester jimhester closed this Sep 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment