Deprecation: General Strategy. We should call the replacement function, right?

[bryanhanson]

I'm making an issue here because it affects all of r-hyperspec/....

To date, I've avoided thinking much about the deprecation process in cbeleites/hyperSpec, hoping I wouldn't have to!

But, lately in the course of giving @sangttruong guidance, I've had a brief look at the deprecation process we currently have. It seems that right now, when a function is deprecated there's a lot of things in the file that get changed (like documentation), and the user gets a nice message about the deprecation with some guidance (all nicely automated thanks to @GegznaV ). However, it looks like the code that runs is the existing code. Shouldn't the program flow be transferred to the replacement function by calling the replacement function? In this case the existing function and unit tests would all be stripped out making a rather small file. And of course, this change would apply only to the develop branch; the master branch must continue to work as it has.

It's possible that calling the replacement function could be made automatic as well by some combination of match.call and do.call -- the function that generates the messages could also call the new code. That would reduce the amount of editing to be done.

Perhaps I am missing something here, I did not study this in great detail. But, I wanted to raise the issue just in case.

[GegznaV]

However, it looks like the code that runs is the existing code. Shouldn't the program flow be transferred to the replacement function by calling the replacement function?

While answering, I think of functions that are renamed in or removed from hyperSpec. If there are some other examples, please, list those functions as I'm not currently aware of them.

[In short] The main idea of mine is (if there are no real issues related to the size of the package) to leave the functionality of the old functions as it is now in order not to break other people's code. And not to make the functionality of the old functions depend on the functionality of the replacement function as, in general, the functionality may differ.

There are some points to consider:

1. [Functionality must not be changed] If I understand correctly, those functions we are talking about, are left in hyperSpec for backward compatibility only and will be removed in 1 year or so after we release the new version of hyperSpec and related packages to CRAN. And when the time comes, we will convert the deprecated functions into defunct functions and at that phase, most of the code could be removed (except informative message). Several more years later the defunct functions may be totally removed from the package.
   Until that, I think the old functions should work as they worked before in order not to break people's code. And the replacement functions must have the possibility to evolve and improve independently from the old functions.

2. [Avoid unnecessary, forbidden dependencies] If a function is replaced by a function that is moved to some other package (like hySpc.ggplot2, hySpc.read.txt, etc.) and we want to use a replacement function in the body of the old function, then hyperSpec must depend on that new package. But the package already depends on hyperSpec. And circular dependencies are not allowed in R. So in this case there is no way to use replacement functions. And the biggest files are those, that contain functions that read data files. But those functions are now moved to the new packages.

3. [Decide individually in each case] There are some functions, which are just renamed and left in hyperSpec. Some of them may use the replacement functions in the body (and some of them, if I recall correctly, already do). But, I think, in most cases, it is better to decide individually if it is safe enough to use the replacement function or not.

Did I answered your question, @bryanhanson? or did you have something more specific in mind?

---

I'm not sure I understand this correctly:

And of course, this change would apply only to the develop branch; the master branch must continue to work as it has.

@bryanhanson, do you mean not changing the master branch after the new version of hyperSpec is released? As we only change this branch when we do CRAN releases.

[bryanhanson]

The reason that I started this issue was a discussion with @cbeleites about avoiding the situation where we have two sets of the same/almost the same code to maintain. Now I am convinced it is a little more nuanced. I'm still thinking this through (be sure to look at #18) but perhaps the best approach is to leave the old code where it is, with the deprecation messages, then release the new hyperSpec and leave it in place for say 3 months. It should easily pass CRAN review, and could co-exist with the new packages. Then, after the 3 months, fully deprecate, meaning the new code is called, or even, the user is just told to go use the new packages.

If we do things this way I still think we should flatten the hyperSpec folder structure and move it to r-hyperspec area.

[bryanhanson]

Bioconductor Deprecation Policy

[GegznaV]

Shouldn't the program flow be transferred to the replacement function by calling the replacement function?

In some cases replacement functions are used in the body of deprecated functions in #331

[cbeleites]

Before going into details for hyperSpec,

• A good guideline is IMHO that there should be no duplicated code within one package. Duplicated code means a maintenance nightmare.
• Copies of functions with the same name are an absolute no-go!
  E.g. right now we have two definitions of function .rubberband(), one overwriting the other.
• Deprecation means: a function (object) is going to be removed.
  This does not mean that the functionality cannot evolve in the usual way. Any acceptable change/evolution in new_function() is also acceptable for a deprecated old.function().

---

I think we have 2 or 3 "patterns" of functions we have deprecated:

1. functions that have been moved outside hyperSpec into the new packages.
   Here we need to keep the functionality in hyperSpec for now. I.e. the deprecated function should contain the working code.
   For the file import functions, I'd say that the code inside hyperSpec should not have any unit tests of the functionality (not even offline), and the deprecation message should warn about this.

2. functions that have been renamed - I'd have explained the deprecation process as:

   1. rename old.function() -> new_function() (and oldfunction.R -> new_function.R)
   2. update all code to use new_function()
   3. update documentation incl. vignettes to use new_function()
   4. build & check (& double-check example code because of possible \dontrun{}) until there are no more errors.
   5. add DEPRECATED-oldfunction.R containing
      • documentation saying that old.function() is deprectated (but not of the functionality any more: that is in the documentation of new_function())
      • definition of old.function <- function (...) which throws the deprecation warning and calls new_function(...)
      • unit test of old.function() that ensures the deprecation warning happens.
   6. run all code we have (vignettes, examples, unit tests, build & check) with options(warn = 2)
      (nothing should happen, this is another check to make sure we did not forget to update any code)

3. the occasional function we want to deprecate that doesn't fit into patterns 1 and 2 (e.g. wc())

To me, deprecated means also:

• no more maintenance of code that still exists under the old function name. For deprecated functions that have (and should have) working code, the deprecation warning should tell people that they use this code at their own risk.
• In most cases for deprecated hyperSpec functions (patterns 1 and 2): no unit tests except making sure that we get the deprecation warning:
  • for the renamed functions, the unit tests of the functionality should be with the new function name.
  • for the file import functions, the unit tests are in the new packages.
    The deprecation warning should clearly state that untested code is used.

---

For the big subset of deprecated functions in hyperSpec that have been renamed within hyperSpec, the new .R file with the definition of old.function() (!) could IMHO either be generated from a template/snippet or maybe dynamically as @bryanhanson suggests. (I'm not sure the necessary work to find out how to dynamically tell Roxygen2 to add the function to the deprecated function list pays off, though? and files following a template may be easier to understand for strangers.(?) )

[GegznaV]

Do I get correctly, that these are the main points we should do/recheck about the deprecated functions:

Copies of functions with the same name are an absolute no-go!

OK, then for the renamed functions that are left in hyperSpec we can remove duplicated code and use the new function in the old function's body (as it is started to be implemented in PR cbeleites/hyperSpec#331. We can continue this work in new PRs as cbeleites/hyperSpec#331 should be merged ASAP to avoid Git conflicts /my fault here/)

For the file import functions, I'd say that the code inside hyperSpec should not have any unit tests of the functionality (not even offline), and the deprecation message should warn about this.

For import functions, that are moved to other packages:

1. we'll remove unit tests and
2. add unit tests that check if the function is deprecated.

For other functions, that are moved to other packages (ggplot2, etc):

1. add unit tests that check if the function is deprecated.

For renamed functions that are in hyperSpec

1. move all other code (incl. unit tests) to the new function's body/file,
2. add unit tests that check if the function is deprecated.

deprecation warning should tell people that they use this code at their own risk.

We'll include this warning in deprecation messages.