Open bryanhanson opened 3 years ago
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; themaster
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.
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.
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
Before going into details for hyperSpec,
.rubberband()
, one overwriting the other.new_function()
is also acceptable for a deprecated old.function()
. I think we have 2 or 3 "patterns" of functions we have deprecated:
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.
functions that have been renamed - I'd have explained the deprecation process as:
old.function()
-> new_function()
(and oldfunction.R
-> new_function.R
)new_function()
new_function()
\dontrun{}
) until there are no more errors.DEPRECATED-oldfunction.R
containing
old.function()
is deprectated (but not of the functionality any more: that is in the documentation of new_function()
)old.function <- function (...)
which throws the deprecation warning and calls new_function(...)
old.function()
that ensures the deprecation warning happens.
options(warn = 2)
the occasional function we want to deprecate that doesn't fit into patterns 1 and 2 (e.g. wc()
)
To me, deprecated means also:
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.(?) )
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:
For other functions, that are moved to other packages (ggplot2
, etc):
For renamed functions that are in hyperSpec
deprecation warning should tell people that they use this code at their own risk.
We'll include this warning in deprecation messages.
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; themaster
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
anddo.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.