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

with_options works as expected #434

Merged
merged 4 commits into from Mar 17, 2014

Conversation

Projects
None yet
2 participants
@krlmlr
Member

krlmlr commented Mar 11, 2014

Didn't work at all. Tests added.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Mar 11, 2014

Just thought of another, perhaps more elegant option:

with_something <- function(set) {
  function(..., code) {
    old <- set(...)
    on.exit(set(old))
    force(code)
  }
}

However, this would require special handling if missing(code) to maintain backward compatibility. Tricky. (?)

@krlmlr

This comment has been minimized.

Member

krlmlr commented Mar 11, 2014

@krlmlr

This comment has been minimized.

Member

krlmlr commented Mar 11, 2014

What is the preferable/intended syntax anyway?

  1. with_options(c(scipen=999), ...)
  2. with_options(scipen=999, ...)

For 2. (my favorite), we can leave with_something as it is, and hard-code with_options or define

with_anything <- function(set) {
  function(..., code) {
    old <- set(...)
    on.exit(set(old))
    force(code)
  }
}
@hadley

This comment has been minimized.

Member

hadley commented Mar 14, 2014

Definitely with_options(c(scipen=999), ...). I can't remember the exact case, but working with ... can be problematic in the restore step (on.exit(set(old)))

@krlmlr

This comment has been minimized.

Member

krlmlr commented Mar 17, 2014

Merged with master, ready for pulling.

I was thinking about moving all of the with_... functionality to a separate package. This paradigm could be useful also outside of package development. Would you support this?

hadley added a commit that referenced this pull request Mar 17, 2014

Merge pull request #434 from krlmlr/with_options
with_options works as expected

@hadley hadley merged commit b2686c9 into r-lib:master Mar 17, 2014

1 check passed

default The Travis CI build passed
Details
@hadley

This comment has been minimized.

Member

hadley commented Mar 17, 2014

Thanks!

@krlmlr krlmlr deleted the krlmlr:with_options branch Oct 6, 2016

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