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

Jump to bottom

Export masks for NSE-only constructs ., J, patterns and measure #6125

Merged
merged 34 commits into from
Jun 20, 2024
Merged

Export masks for NSE-only constructs ., J, patterns and measure #6125

merged 34 commits into from
Jun 20, 2024

Conversation

Nj221102
Copy link
Contributor

@Nj221102 Nj221102 commented May 6, 2024 •
edited
Loading

This pull request addresses issue #5604 and #5277 , which aims to export masks for all non-standard evaluation (NSE) only constructs within [ and melt() functions. Currently, only .N, .I, .GRP, .GRPI, .SD, and := are exported, but this PR extends the export functionality to include.,J, measure and patterns.

  1. Masked NSE constructs as function(...) NULL.
  2. exported the functions in Namespace.
  3. changed one test .

This PR closes #5604
closes #5277

@Nj221102 Nj221102 requested review from tdhock and Anirban166 May 6, 2024 17:17
@github-actions GitHub Actions
Copy link

github-actions bot commented May 6, 2024 •
edited
Loading

Comparison Plot

Generated via commit ea84607

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 19 seconds

@tdhock
Copy link
Member

tdhock commented May 8, 2024

please edit your first comment in this PR which says "This PR closes #5604 #5277." (two issues linked on one line)
please write one "Closes " per line so both issues will be closed when this PR is merged (right now only the first will be, because they are on the same line)
you can check that under Development in the right side of the screen (right now shows only one issue "Export..." but should show two)

tdhock
tdhock requested changes May 8, 2024
View reviewed changes
R/data.table.R Outdated Show resolved Hide resolved
@Nj221102 Nj221102 requested a review from tdhock May 9, 2024 10:25
tdhock
tdhock requested changes May 10, 2024
View reviewed changes
R/data.table.R Outdated Show resolved Hide resolved
@Nj221102 Nj221102 mentioned this pull request May 29, 2024
Open
23 tasks
@Nj221102 Nj221102 marked this pull request as draft May 30, 2024 11:37
@Nj221102
Copy link
Contributor Author

Nj221102 commented Jun 1, 2024

@tdhock since their is no need of masks for pattern and measure, how should we go about solving the problem in #5277 , should we export them regardless without masks or just change documentation to inform that these functions aren't being exported.

@Nj221102 Nj221102 requested a review from tdhock June 1, 2024 21:01
tdhock
tdhock reviewed Jun 3, 2024
View reviewed changes
NAMESPACE Outdated Show resolved Hide resolved
tdhock
tdhock requested changes Jun 3, 2024
View reviewed changes
R/data.table.R Outdated Show resolved Hide resolved
@Nj221102 Nj221102 requested a review from tdhock June 3, 2024 03:55
tdhock
tdhock requested changes Jun 3, 2024
View reviewed changes
Copy link
Member

@tdhock tdhock left a comment

Choose a reason for hiding this comment

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

can you please add a NEWS item?

@Nj221102
Copy link
Contributor Author

Nj221102 commented Jun 3, 2024

done :)

@Nj221102 Nj221102 requested a review from tdhock June 3, 2024 13:42
@Nj221102 Nj221102 marked this pull request as ready for review June 3, 2024 13:42
tdhock
tdhock reviewed Jun 3, 2024
View reviewed changes
NEWS.md Outdated Show resolved Hide resolved
@Nj221102 Nj221102 requested a review from tdhock June 19, 2024 08:35
tdhock
tdhock requested changes Jun 19, 2024
View reviewed changes
R/data.table.R Outdated Show resolved Hide resolved
R/data.table.R Outdated Show resolved Hide resolved
@Nj221102 Nj221102 requested a review from tdhock June 19, 2024 15:51
tdhock
tdhock requested changes Jun 19, 2024
View reviewed changes
R/data.table.R Outdated Show resolved Hide resolved
R/data.table.R Outdated Show resolved Hide resolved
tdhock
tdhock approved these changes Jun 20, 2024
View reviewed changes
Copy link
Member

@tdhock tdhock left a comment

Choose a reason for hiding this comment

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

ok looks good to me but please wait for a second review from @MichaelChirico @jangorecki or @ben-schwen

MichaelChirico
MichaelChirico reviewed Jun 20, 2024
View reviewed changes
NAMESPACE
@@ -27,7 +27,7 @@ export(tstrsplit)
export(frank)
export(frankv)
export(address)
export(.SD,.N,.I,.GRP,.NGRP,.BY,.EACHI)
export(.SD,.N,.I,.GRP,.NGRP,.BY,.EACHI, ., J, measure, patterns)
Copy link
Member

Choose a reason for hiding this comment

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

warning: I imagine this (.) will break some revdeps. Let's see.

MichaelChirico
MichaelChirico reviewed Jun 20, 2024
View reviewed changes
NEWS.md Outdated Show resolved Hide resolved
@Nj221102 @MichaelChirico
Update NEWS.md
5ebd8b6 
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
MichaelChirico
MichaelChirico reviewed Jun 20, 2024
View reviewed changes
R/data.table.R Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico reviewed Jun 20, 2024
View reviewed changes
R/data.table.R Outdated Show resolved Hide resolved
R/data.table.R Outdated Show resolved Hide resolved
vignettes/datatable-importing.Rmd Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
Nj221102 and others added 2 commits June 20, 2024 11:37
@Nj221102 @MichaelChirico
Update vignettes/datatable-importing.Rmd
127b45e 
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
@Nj221102 @MichaelChirico
Update NEWS.md
90cd873 
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
MichaelChirico
MichaelChirico reviewed Jun 20, 2024
View reviewed changes
NEWS.md Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico approved these changes Jun 20, 2024
View reviewed changes
Copy link
Member

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

Thank you!

@MichaelChirico MichaelChirico merged commit fab93a9 into master Jun 20, 2024
4 checks passed
@MichaelChirico MichaelChirico deleted the Masking-functions branch June 20, 2024 06:27
@tdhock tdhock mentioned this pull request Jun 21, 2024
Closed
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdhock tdhock tdhock approved these changes

@MichaelChirico MichaelChirico MichaelChirico approved these changes

@Anirban166 Anirban166 Awaiting requested review from Anirban166

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Export masks for all NSE-only constructs (currently at least '.', 'J', 'patterns') patterns function missing
3 participants
@Nj221102 @tdhock @MichaelChirico