remove most usages of <- #3592
remove most usages of <- #3592
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3592 +/- ##
==========================================
+ Coverage 97.63% 97.69% +0.06%
==========================================
Files 66 66
Lines 12721 12721
==========================================
+ Hits 12420 12428 +8
+ Misses 301 293 -8
Continue to review full report at Codecov.
|
R/data.table.R
Outdated
| @@ -1151,7 +1151,7 @@ replace_dot_alias <- function(e) { | |||
| # Note that the NAMED(dt)>1 doesn't work because .Call | |||
| # always sets to 2 (see R-ints), it seems. Work around | |||
| # may be possible but not yet working. When the NAMED test works, we can drop allocwarn argument too | |||
| # because that's just passed in as FALSE from [<- where we know `*tmp*` isn't really NAMED=2. | |||
| # because that's just passed in as FALSE from [= where we know `*tmp*` isn't really NAMED=2. | |||
There was a problem hiding this comment.
I see three places where you've changed [<- to [=, and two with
There was a problem hiding this comment.
good eye, thanks. shouldn't be any in actual code (tests compile & run)
the key is if you use it in actual code, it's gotta be backticked or quoted (`[<-` or "[<-"), which is caught by the regex
There was a problem hiding this comment.
Anyway the regex could be fixed, previous character should be not-[<[]
| @@ -90,9 +90,11 @@ round.IDate <- function (x, digits=c("weeks", "months", "quarters", "years"), .. | |||
| (setattr(as.integer(unclass(e1) + unclass(e2)), "class", c("IDate", "Date"))) # () wrap to return visibly | |||
| } | |||
|
|
|||
| `-.IDate` <- function (e1, e2) { | |||
| if (!inherits(e1, "IDate")) | |||
| `-.IDate` = function (e1, e2) { | |||
There was a problem hiding this comment.
this part of the overall diff is not just <- removal... made this slight change in the second commit (coverage).
I was a bit wary of dispatching to -.Date since the result could then be Date. But the if ( isReallyReal(e2) ) branch below does the same.
MichaelChirico
force-pushed
the
purge_arrow_assign
branch
from
5a94c8e
to
a071aa7
Compare
MichaelChirico
force-pushed
the
purge_arrow_assign
branch
from
2471441
to
98b66b1
Compare
|
I was just fixing those new tests too as I thought you'd be asleep :-) |
inst/tests/tests.Rraw
Outdated
| ## passing Date to second argument of as.POSIXct.ITime | ||
| t = as.ITime(0L) | ||
| test(2051.5, as.POSIXct(t, .Date(0L)), .POSIXct(0, 'UTC')) | ||
|
|
There was a problem hiding this comment.
@MichaelChirico Please can you remove these 4 blank lines. The style in the file is that a group of tests like 2051.1 - 2051.6 should appear together as a block. (I was just doing that but since you're awake and making changes right now, otherwise I'm find to tidy things like that.)
There was a problem hiding this comment.
Usually yes, in this case, it's coverage tests & the file being covered is different (e.g. 2051.3 is from print.data.table.R while rleidv is from data.table.R). Doesn't feel like a new test number is needed but some visual space made sense. can revert though
There was a problem hiding this comment.
It's a nice single block of coverage tests to my eyes. Which is in an unrelated PR -- which is more to the point. In a wide-ranging very high line number PR like this, I was hoping to see there were no changes to tests at all. But then I saw there were changes to tests. But then I saw it was just a bit of extra coverage in a single block, so that's ok. That's the way I looked at it.
There was a problem hiding this comment.
Yea, makes sense. The coverage stuff is because Codecov complained on first commit since the changes hit some uncovered lines so the patch diff failed, i.e., there's a reason I threw those tests into this PR in particular. So I took a pass at some lines that stuck out in a scan of the coverage diff.
I made sure to file the coverage stuff in a different commit, so in git history at least it's easier to see. But if anyone looks at this PR as a source, harder to figure out what's going on.
There was a problem hiding this comment.
Oh I see now, thanks. Makes perfect sense.
Co-Authored-By: Frank <franknarf1@users.noreply.github.com>
…n't have .Date and probably best not to use documented-internal .Date anyway
Closes #3590
On the way home I thought of a simple regex to accomplish this, after a few tweaks it's still pretty simple (this is for mac
sedso the command may need adjustment on other platforms):[^(]+ensures it's not defined within-line as a function argument;[^<[)ensure it's not[<-,<<-, or()<-.[^."`]covers cases like"key<-"anddimnames<-.data.table.There's one false positive (
fread.R):^ this is defining
funfor thewarningfortryCatchwhich is then recycled toerrorAfter this
grep -r "<-" R | wc -lis down to 145 from 628.There are some false negatives:
Some of these can be hit with a regex targeting the part of the line after
<-:After this we're down to
grep -r "<-" R | wc -l= 135Still a small number of false negatives, fine to ignore. 135 is still tediously large to do by hand & most are true positives.