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

Change branch's 'merge' argument to 'continue' #57

Closed
Enchufa2 opened this Issue Jun 4, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@Enchufa2
Member

Enchufa2 commented Jun 4, 2016

I was thinking, @Bart6114, that we chose a not-so-intuitive name for branch's merge argument. It would be much better something like continue, but I don't see an easy way to change this without breaking compatibility. Any ideas?

@Enchufa2 Enchufa2 added the question label Jun 4, 2016

@Enchufa2

This comment has been minimized.

Member

Enchufa2 commented Jun 4, 2016

After some digging, this seems to work:

# roxygen documentation here
# @export
somefunc <- function(continue, x) {
  x * continue
}

# save the latter
somefunc.call <- somefunc
# intercept it
somefunc <- function(...) {
  # get call
  caller <- sys.call()
  # replace the function name
  caller[[1]] <- as.name("somefunc.call")
  # replace "continue" and show a warning if "merge" is found
  if ("merge" %in% names(caller)) {
    warning("'merge' is deprecated, use 'continue' instead")
    caller$continue <- caller$merge
    caller$merge <- NULL
  }
  # make the proper call
  eval(caller)
}

somefunc(continue=2, 3)
#> [1] 6
somefunc(2, 3)
#> [1] 6
somefunc(merge=2, 3)
#> [1] 6
#> Warning message:
#> In somefunc(merge = 2, 3) : 'merge' is deprecated, use 'continue' instead

What do you think?

@Bart6114

This comment has been minimized.

Member

Bart6114 commented Jun 10, 2016

@Enchufa2, fully agree with the more intuitive naming.

What about the below snippet?

branch <- function(traj, option, continue=merge, merge=continue, ...) traj$branch(option, continue, ...)

We could then simply add to the documentation that from version x, the support for the merge parameter would be dropped (and maybe show a warning).

@Enchufa2

This comment has been minimized.

Member

Enchufa2 commented Jun 11, 2016

If you are ok with the renaming, I'd prefer my scheme above. The problem is that your snippet breaks backwards compatibility. With my example, however,

  • The new parameter works.
  • The unnamed call works.
  • The old parameter works and shows a warning.

The documentation can still describe the old parameter, saying that from version x, the support for it would be dropped.

@Enchufa2 Enchufa2 added enhancement and removed question labels Jun 11, 2016

@Enchufa2 Enchufa2 changed the title from Branch's 'merge' argument to Change branch's 'merge' argument to 'continue' Jun 11, 2016

@Enchufa2

This comment has been minimized.

Member

Enchufa2 commented Jun 11, 2016

Mmmmh... but my approach has problems with magrittr pipelines...

@Bart6114

This comment has been minimized.

Member

Bart6114 commented Jun 11, 2016

What about the below?

the_func<-function(new=old, old="_deprecated"){
  if(!old=="_deprecated") warning("use of 'old' is deprecated")
  new
}

# new param works
the_func(new=123)
# unnamed call works
the_func(123)
# old works and shows a warning
the_func(old=123)

Haven't compared it, but I think that this might also be more performant.

@Enchufa2 Enchufa2 added this to the v3.3.0 milestone Jun 12, 2016

@Enchufa2 Enchufa2 closed this in 8a4e0a9 Jun 12, 2016

@Enchufa2

This comment has been minimized.

Member

Enchufa2 commented Jun 12, 2016

There was a problem when the old param was used, but it's solved. Done, thanks!

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