Skip to content
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.

Sign up for GitHub

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

Removing the export of masks for . and j #6198

Merged
merged 9 commits into from
Jun 23, 2024
Merged

Removing the export of masks for . and j #6198

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c164809
removing problematic masks
Jun 22, 2024
db788a1
small changes
Jun 22, 2024
f6c1390
Update NAMESPACE
Nj221102 Jun 23, 2024
b85f6a9
Update R/data.table.R
Nj221102 Jun 23, 2024
cbb6c39
Update NEWS.md
Nj221102 Jun 23, 2024
bc9af4c
updated test
Jun 23, 2024
6167db2
updated test
Jun 23, 2024
04813e9
updated test
Jun 23, 2024
675865c
Merge branch 'master' into masks
MichaelChirico Jun 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion NAMESPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export(tstrsplit)
export(frank)
export(frankv)
export(address)
export(.SD,.N,.I,.GRP,.NGRP,.BY,.EACHI, ., J, measure, patterns)
export(.SD,.N,.I,.GRP,.NGRP,.BY,.EACHI, measure, patterns)
Nj221102 marked this conversation as resolved.
Show resolved Hide resolved
export(rleid)
export(rleidv)
export(rowid)
Expand Down
3 changes: 2 additions & 1 deletion NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@

16. `print.data.table` gains new argument `show.indices` and option `datatable.show.indices` that allows the user to print a `data.table`'s indices as columns without having to modify the `data.table` itself. Thanks @MichaelChirico for the report and @joshhwuu for the PR.

17. `.`, `J`, `measure`, and `patterns` are now exported for use within `[` and `melt()`, for consistency with other NSE exports like `.N` and `:=`, [#5604](https://github.com/Rdatatable/data.table/issues/5604). Package developers can now import these names to avoid `R CMD check` `NOTE`s about them being undefined variables. Thanks to @MichaelChirico and @ylelkes for the suggestions and @Nj221102 for implementing.
17. The `measure` and `patterns` functions are now exported for use within `[` and `melt()` to ensure consistency with other non-standard evaluation (NSE) exports like `.N` and `:=`. This change addresses [#5604](https://github.com/Rdatatable/data.table/issues/5604), allowing package developers to import these names and avoid `R CMD check` `NOTE`s about undefined variables. Temporarily delayed the export of `.` and `j` to resolve revdep issues. We plan to revisit and potentially re-export these symbols once the issues are resolved. Thanks to @MichaelChirico and @ylelkes for their suggestions, and to @Nj221102 for the implementation.
Nj221102 marked this conversation as resolved.
Show resolved Hide resolved


## TRANSLATIONS

Expand Down
1 change: 1 addition & 0 deletions R/data.table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2770,6 +2770,7 @@ address = function(x) .Call(Caddress, eval(substitute(x), parent.frame()))
stopf('Check that is.data.table(DT) == TRUE. Otherwise, :=, `:=`(...) and let(...) are defined for use in j, once only and in particular ways. See help(":=").')
}

# TODO: re-export the `.` and `j` functions once revdep issues are resolved.
Nj221102 marked this conversation as resolved.
Show resolved Hide resolved
J = function(...) {
ben-schwen marked this conversation as resolved.
Show resolved Hide resolved
stopf("J() called outside of [.data.table. J() is only intended for use in i.")
}
Expand Down
2 changes: 1 addition & 1 deletion inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.")

Copy link
Contributor Author

@Nj221102 Nj221102 Jun 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

oh i am sorry , i didn't notice 😓

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, plz check it out

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Nj221102 Nj221102 Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# Test get in j
DT = data.table(a=1:3,b=4:6)
Expand Down
Loading