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.

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

feat : added condition to check the RHS of := #6742

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

venom1204
Copy link
Contributor

@venom1204 venom1204 commented Jan 20, 2025

Closes #5829.
I have added a condition to validate the right-hand side (RHS) of the := operator. If the RHS is a standalone function, an error is raised with an informative message guiding the user to wrap the function in a list if it is intended to be stored.

@tdhock, could you please review this and let me know for further improvements or additional scenarios that I should do?thank you.

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.58%. Comparing base (d45546b) to head (b333d52).

Files with missing lines Patch % Lines
R/data.table.R 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6742      +/-   ##
==========================================
- Coverage   98.59%   98.58%   -0.01%     
==========================================
  Files          79       79              
  Lines       14664    14666       +2     
==========================================
+ Hits        14458    14459       +1     
- Misses        206      207       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

also please add test cases based on my issue

@@ -316,6 +316,9 @@ replace_dot_alias = function(e) {
warningf("nomatch isn't relevant together with :=, ignoring nomatch")
nomatch=NULL
}
if (length(jsub) >= 3L && is.function(jsub[[3L]])) {
stopf("the right hand side of := should be a vector, list, data.frame, function or call, not a function,to store a function wrap it in a list: DT[, newcol := list(myfun)]")
Copy link
Member

Choose a reason for hiding this comment

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

this is self-contradictory "should be a ...function or call, not a function..." please clarify/fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual error message will be:
"The right-hand side of := should be a vector, list, or data.frame, not a function. To store a function, wrap it in a list: DT[, newcol := list(myfun)]"

Can you confirm if this implementation is correct? When I run the test to check it, the following error is displayed:
Test 2306.2 produced 1 errors but expected 0
Expected:
Observed: invalid type/length (closure/3) in vector allocation

This error is for the test:

Edit
DT = data.table(x = 1:3)
test(2306.2, DT[, y := list(mean)], data.table(x = 1:3, y = list(mean, mean, mean)))

I cannot figure out where this observed error is coming from.

Choose a reason for hiding this comment

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

if (length(jsub) >= 3L && is.function(jsub[[3L]])) {
    stopf("the right hand side of := should be a vector, list, data.frame, or function call. To store a raw function, wrap it in a list: DT[, newcol := list(myfun)]")
}

the intent should be:

  • Raw functions shouldn't be assigned directly as column values
  • Functions should be wrapped in a list when being assigned
  • Function calls (like mean(x)) are allowed

Choose a reason for hiding this comment

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

@venom1204 fix at your own convenience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

various errors when RHS of := is closure/function
3 participants