yihui / xfun

Miscellaneous R functions
https://yihui.org/xfun/
Other
135 stars 28 forks source link

Add Depends: R (>= 3.5.0)? #76

Closed HenrikBengtsson closed 1 year ago

HenrikBengtsson commented 1 year ago

I see that NEWS for xfun 0.37 says:

However, searching the code](https://github.com/search?q=repo%3Ayihui%2Fxfun%20isFALSE&type=code), xfun already now uses base::isFALSE() in several places, so, it looks like it already now requires R (>= 3.5.0).

yihui commented 1 year ago

Yes, base::isFALSE is used in four places. I'll replace it with identical(x, FALSE). Thanks!

mmaechler commented 1 year ago

eehmm.. did I recommend that? isFALSE(x) is really not the same as !isTRUE(x)

yihui commented 1 year ago

@mmaechler Yes, I know they are not equivalent. I promise I'll be careful :) I didn't meant I'd blindly search and replace. Thanks!

yihui commented 1 year ago

I've replaced them with identical(x, FALSE). Thanks again!

mmaechler commented 1 year ago

Hmm.. I don't get it. We (R-core) introduced the current isFALSE to base because it is more correct than identical(., FALSE) which is also slower, BTW.

Why on earth do you replace it?
The whole issue was only about the R (>= 3.5.0) vs some other versions of R, no? If the replacement is only for really old R versions, then replace isFALSE by its current definition. ... I have done that for many years in some of my packages: Something like

if(getRversion() < "3.5.0")
  isFALSE <- function (x)  is.logical(x) && length(x) == 1L && !is.na(x) && !x

and then use isFALSE(....) everywhere

yihui commented 1 year ago

Why on earth do you replace it?

@mmaechler A few reasons: 1) I don't want to require R >= 3.5.0 (yet) because none of other functions require it in this package; 2) I regret having introduced isFALSE() to xfun in the beginning and have wished to remove it. Yes, I know I could define it only for R < 3.5.0, but I don't think many people still use that old version of R, so it's probably not worth the effort, and I also want to push users to base::isFALSE(). Copying code from base R also means I need to add R core as ctb to this package, and I don't want to worry about whether MIT and GPL could mix well. 3) Yes, base::isFLASE() is more correct and faster, but the correctness and speed don't really matter much in this package (e.g., 1 vs 2 microseconds). In the long run, I'll also switch to using base::isFALSE() in this package (I have done it in some other packages). It will happen someday, but just not right now.

In summary, xfun::isFALSE() shouldn't have existed in the first place, but unfortunately it existed. Deprecating a function takes time, because we don't know how many people are still using it. I don't want someone to suddenly realize isFALSE() is no longer present in xfun but doesn't know what to do. That's why we need to show the deprecation message for as long as possible, which contains instructions on what to do (in this case, use base::isFALSE()).

I completely understand your concerns and confusion, and very much appreciate your advice! Don't worry. I promise I'll continue to teach the woRld that base::isFALSE() is the one and only correct function to call whenever I see any use of xfun::isFALSE() in future :)