Removing the export of masks for `.` and `j` #6198

Nj221102 · 2024-06-22T09:58:46Z

Description

Reverting export of . and J due to them causing revdep issues.

Related issue
#6197

github-actions · 2024-06-22T10:15:30Z

Generated via commit 675865c

Download link for the artifact containing the test results: ↓ atime-results.zip

Time taken to finish the standard R installation steps: 11 minutes and 49 seconds

Time taken to run atime::atime_pkg on the tests: 3 minutes and 17 seconds

MichaelChirico

please also add a TODO about reverting this later as a reminder in the codebase, and write something in the NEWS about the intention to later export . and J

R/data.table.R

NAMESPACE

Co-authored-by: Michael Chirico <[email protected]>

NEWS.md

R/data.table.R

MichaelChirico · 2024-06-23T04:53:19Z

inst/tests/tests.Rraw

@@ -2186,7 +2186,7 @@ if (ncol(DT)==2L) setnames(DT,c("A","B")) # else don't stop under torture with s
 test(714, DT[,z:=6:10], data.table(A=1:5,B=5,z=6:10))

 # Test J alias is now removed outside DT[...] from v1.8.7 (to resolve rJava::J conflict)
-test(715, J(a=1:3,b=4), error="J() called outside of [.data.table. J() is only intended for use in i.")
+test(715, J(a=1:3,b=4), error=base_messages$missing_function("J"))


Suggested change

test(715, J(a=1:3,b=4), error=base_messages$missing_function("J"))

test(715, J(a=1:3,b=4), error=base_messages$missing_function("J"))

# future:

# test(715, J(a=1:3,b=4), error="J() called outside of [.data.table. J() is only intended for use in i.")

this will cause an error because we aren't exporting J

Test 715 didn't produce the correct error: Expected: J() called outside of [.data.table. J() is only intended for use in i. Observed: could not find function "J"

so i added j to list where internal-only non exposed functions are imported

haszlib = data.table:::haszlib J = data.table:::J

is this better or just not changing error ?

sorry, I think you misread the suggested edit -- in mine the "new" test is commented out.

sorry, I think you misread the suggested edit -- in mine the "new" test is commented out.

oh i am sorry , i didn't notice 😓

done, plz check it out

I don't know how this passed, but test 715 is now failing locally on my system and I think it should have failed in our CI too.

Since we leave the J function in line 2773 the behavior should already be the future behavior?

@ben-schwen I don't know what's happening but current master branch doesn't seem to be failing test 715 in my local system,

All 11202 tests (last 2264.8) in tests/tests.Rraw completed ok in 14.6s elapsed (14.2s cpu)

Since we leave the J function in line 2773 the behavior should already be the future behavior?

sorry i don't understand much, are you saying that J should be behaving as defined in data.table.R ? we aren't exporting J from our Namespace nor we import it where internal-only non exposed functions are imported in tests.Rraw , so AFAIK i don't think tests.Rraw have access to function j hence the error error=base_messages$missing_function("J")

Hi @ben-schwen are you still facing this issue, plz inform me if I can be of help :)

Co-authored-by: Michael Chirico <[email protected]>

MichaelChirico

Thank you!

MichaelChirico · 2024-06-24T13:36:03Z

oh, it's failing under cc(). let's comment it out in our namespace.

…

On Mon, Jun 24, 2024, 4:05 AM Nitish Jha ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In inst/tests/tests.Rraw <#6198 (comment)> : > @@ -2186,7 +2186,7 @@ if (ncol(DT)==2L) setnames(DT,c("A","B")) # else don't stop under torture with s test(714, DT[,z:=6:10], data.table(A=1:5,B=5,z=6:10)) # Test J alias is now removed outside DT[...] from v1.8.7 (to resolve rJava::J conflict) -test(715, J(a=1:3,b=4), error="J() called outside of [.data.table. J() is only intended for use in i.") +test(715, J(a=1:3,b=4), error=base_messages$missing_function("J")) Hi @ben-schwen <https://github.com/ben-schwen> are you still facing this issue, plz inform me if I can be of help :) — Reply to this email directly, view it on GitHub <#6198 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2BA5ITDEMISHT3QGNKNBDZI74O5AVCNFSM6AAAAABJXJZHJGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMZVGQ3DGNZZGQ> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

Nj221102 · 2024-06-24T22:36:45Z

oh, it's failing under cc(). let's comment it out in our namespace.
…
On Mon, Jun 24, 2024, 4:05 AM Nitish Jha @.> wrote: @.* commented on this pull request. ------------------------------ In inst/tests/tests.Rraw <#6198 (comment)> : > @@ -2186,7 +2186,7 @@ if (ncol(DT)==2L) setnames(DT,c("A","B")) # else don't stop under torture with s test(714, DT[,z:=6:10], data.table(A=1:5,B=5,z=6:10)) # Test J alias is now removed outside DT[...] from v1.8.7 (to resolve rJava::J conflict) -test(715, J(a=1:3,b=4), error="J() called outside of [.data.table. J() is only intended for use in i.") +test(715, J(a=1:3,b=4), error=base_messages$missing_function("J")) Hi @ben-schwen https://github.com/ben-schwen are you still facing this issue, plz inform me if I can be of help :) — Reply to this email directly, view it on GitHub <#6198 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB2BA5ITDEMISHT3QGNKNBDZI74O5AVCNFSM6AAAAABJXJZHJGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMZVGQ3DGNZZGQ . You are receiving this because you modified the open/close state.Message ID: @.***>

Isn't export of j already commented out in Namespace.

export(.SD,.N,.I,.GRP,.NGRP,.BY,.EACHI, measure, patterns)
# TODO(#6197): Export these.
# export(., J)

Am i missing something here?

MichaelChirico · 2024-06-24T22:39:50Z

yes, but cc() is common for data.table development, it in effect attaches the full data.table namespace, thus unexported functions are also defined & visible.

…

On Mon, Jun 24, 2024, 3:37 PM Nitish Jha ***@***.***> wrote: oh, it's failing under cc(). let's comment it out in our namespace. … <#m_5153630067027332990_> On Mon, Jun 24, 2024, 4:05 AM Nitish Jha *@*.*> wrote: @.** commented on this pull request. ------------------------------ In inst/tests/tests.Rraw <#6198 (comment) <#6198 (comment)>> : > @@ -2186,7 +2186,7 @@ if (ncol(DT)==2L) setnames(DT,c("A","B")) # else don't stop under torture with s test(714, DT[,z:=6:10], data.table(A=1:5,B=5,z=6:10)) # Test J alias is now removed outside DT[...] from v1.8.7 (to resolve rJava::J conflict) -test(715, J(a=1:3,b=4), error="J() called outside of [.data.table. J() is only intended for use in i.") +test(715, J(a=1:3,b=4), error=base_messages$missing_function("J")) Hi @ben-schwen <https://github.com/ben-schwen> https://github.com/ben-schwen are you still facing this issue, plz inform me if I can be of help :) — Reply to this email directly, view it on GitHub <#6198 (comment) <#6198 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB2BA5ITDEMISHT3QGNKNBDZI74O5AVCNFSM6AAAAABJXJZHJGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMZVGQ3DGNZZGQ . You are receiving this because you modified the open/close state.Message ID: *@*.***> Isn't export of j already commented out in Namespace. export(.SD,.N,.I,.GRP,.NGRP,.BY,.EACHI, measure, patterns) # TODO(#6197): Export these. # export(., J) Am i missing something here? — Reply to this email directly, view it on GitHub <#6198 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2BA5P5JMXGS45GPIPE363ZJCNRJAVCNFSM6AAAAABJXJZHJGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBXGUZTONRUHA> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

MichaelChirico · 2024-06-24T22:41:17Z

also to clarify, I mean to comment out the definition in the .R file (which builds the package namespace), not the NAMESPACE file, which defines package exports (and is already commented out as you said)

Nj221102 · 2024-06-24T23:26:36Z

also to clarify, I mean to comment out the definition in the .R file (which builds the package namespace), not the NAMESPACE file, which defines package exports (and is already commented out as you said)

Thanks for explanation :)

removing problematic masks

c164809

Nj221102 requested a review from MichaelChirico as a code owner June 22, 2024 09:58

Nj221102 requested review from tdhock, MichaelChirico and Anirban166 and removed request for MichaelChirico June 22, 2024 09:59

MichaelChirico reviewed Jun 22, 2024

View reviewed changes

R/data.table.R Show resolved Hide resolved

small changes

db788a1

Anirban166 approved these changes Jun 22, 2024

View reviewed changes

Nj221102 requested a review from MichaelChirico June 22, 2024 23:40

MichaelChirico reviewed Jun 23, 2024

View reviewed changes

NAMESPACE Show resolved Hide resolved

Update NAMESPACE

f6c1390

Co-authored-by: Michael Chirico <[email protected]>

MichaelChirico reviewed Jun 23, 2024

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

MichaelChirico reviewed Jun 23, 2024

View reviewed changes

R/data.table.R Outdated Show resolved Hide resolved

MichaelChirico reviewed Jun 23, 2024

View reviewed changes

Nj221102 and others added 4 commits June 23, 2024 10:26

Update R/data.table.R

b85f6a9

Co-authored-by: Michael Chirico <[email protected]>

Update NEWS.md

cbb6c39

Co-authored-by: Michael Chirico <[email protected]>

updated test

bc9af4c

updated test

6167db2

Nj221102 requested a review from MichaelChirico June 23, 2024 05:20

updated test

04813e9

MichaelChirico approved these changes Jun 23, 2024

View reviewed changes

Merge branch 'master' into masks

675865c

MichaelChirico merged commit 4e6ad97 into master Jun 23, 2024
4 checks passed

MichaelChirico deleted the masks branch June 23, 2024 14:23

Nj221102 mentioned this pull request Jun 27, 2024

Comment Out Definitions of J and . to Fix Test 715 in cc() Environment #6207

Merged

MichaelChirico mentioned this pull request Jul 11, 2024

revdep issues caused when . and J are exported #6197

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing the export of masks for `.` and `j` #6198

Removing the export of masks for `.` and `j` #6198

Nj221102 commented Jun 22, 2024 •

edited by MichaelChirico

Loading

github-actions bot commented Jun 22, 2024 •

edited

Loading

MichaelChirico left a comment

MichaelChirico Jun 23, 2024

Nj221102 Jun 23, 2024 •

edited

Loading

MichaelChirico Jun 23, 2024

Nj221102 Jun 23, 2024

Nj221102 Jun 23, 2024

ben-schwen Jun 24, 2024

Nj221102 Jun 24, 2024 •

edited

Loading

Nj221102 Jun 24, 2024

MichaelChirico left a comment

MichaelChirico commented Jun 24, 2024 via email

Nj221102 commented Jun 24, 2024

MichaelChirico commented Jun 24, 2024 via email

MichaelChirico commented Jun 24, 2024

Nj221102 commented Jun 24, 2024

Removing the export of masks for . and j #6198

Removing the export of masks for . and j #6198

Conversation

Nj221102 commented Jun 22, 2024 • edited by MichaelChirico Loading

github-actions bot commented Jun 22, 2024 • edited Loading

MichaelChirico left a comment

Choose a reason for hiding this comment

MichaelChirico Jun 23, 2024

Choose a reason for hiding this comment

Nj221102 Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

MichaelChirico Jun 23, 2024

Choose a reason for hiding this comment

Nj221102 Jun 23, 2024

Choose a reason for hiding this comment

Nj221102 Jun 23, 2024

Choose a reason for hiding this comment

ben-schwen Jun 24, 2024

Choose a reason for hiding this comment

Nj221102 Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Nj221102 Jun 24, 2024

Choose a reason for hiding this comment

MichaelChirico left a comment

Choose a reason for hiding this comment

MichaelChirico commented Jun 24, 2024 via email

Nj221102 commented Jun 24, 2024

MichaelChirico commented Jun 24, 2024 via email

MichaelChirico commented Jun 24, 2024

Nj221102 commented Jun 24, 2024

Removing the export of masks for `.` and `j` #6198

Removing the export of masks for `.` and `j` #6198

Nj221102 commented Jun 22, 2024 •

edited by MichaelChirico

Loading

github-actions bot commented Jun 22, 2024 •

edited

Loading

Nj221102 Jun 23, 2024 •

edited

Loading

Nj221102 Jun 24, 2024 •

edited

Loading