-
Notifications
You must be signed in to change notification settings - Fork 968
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see three places where you've changed [<- to [=, and two with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
5a94c8e
to
a071aa7
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
sed
so 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
fun
for thewarning
fortryCatch
which is then recycled toerror
After this
grep -r "<-" R | wc -l
is 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.