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

Enhance Error Message for using := or let in Non-data.table-aware Environment #6019

Merged
merged 7 commits into from
Mar 25, 2024

Conversation

Nj221102
Copy link
Contributor

Description:

This PR addresses issue #5721. The current error message generated when := is used directly in an environment not aware of data.table (cedta() returns FALSE) can be misleading. This PR enhances the error message to provide more specific guidance to the user.

Changes Made:

  • Added a check in the [.data.table function to detect the presence of := or let in the j argument.
  • If := or let is detected, a more specific error message is thrown, advising the user to ensure that data.table is imported and used explicitly in their package.

Proposed Error Message:

"[ was called on a data.table in an environment that is not data.table-aware (i.e. cedta()), but ':=' or 'let' was used, implying the owner of this call really intended for data.table methods to be called. See vignette('datatable-importing') for details on properly importing data.table."

Closes #5721

Copy link

codecov bot commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.53%. Comparing base (b4ae3ef) to head (dadcd59).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6019   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files          80       80           
  Lines       14913    14915    +2     
=======================================
+ Hits        14545    14547    +2     
  Misses        368      368           

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

@Nj221102
Copy link
Contributor Author

Can someone suggest, how to write tests for this error ? , this error only occurs in not data.table aware enviroment(cedta() == false) , how to mimic such environment in tests, I have tried searching for it , wasn't able to find anything helpful.

R/data.table.R Outdated
@@ -145,6 +145,10 @@ replace_dot_alias = function(e) {
# the drop=NULL is to sink drop argument when dispatching to [.data.frame; using '...' stops test 147
if (!cedta()) {
# Fix for #500 (to do)
if (any(grepl(":=|let", substitute(j)))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There was some discussion about tidyverse and the rlang version of :=. Another version to consider would be this :

if (substitute(j) %iscall% c(":=", "let"))

I also researched tidyverse. The above implementation should be faster and more reliable than original proposal, but it does seem unlikely for us to encounter the way := is used in dplyr.

https://dplyr.tidyverse.org/articles/programming.html

@ColeMiller1
Copy link
Contributor

As for testing, I believe there is at least one existing test that appears to be somewhat indirect:

test(14.1, {example(':=', package='data.table', local=TRUE, echo=FALSE); TRUE})

Based on #5654 , we seem to have more options if we wanted to. This triggers an error when I have data.table loaded. We would want to clean up the flag if implemented in tests. Note, I am not on this branch so I get the old message.

.datatable.aware = TRUE
dt = data.table(a = 1L)
dt[, b:=2L]

## Error: Check that is.data.table(DT) == TRUE. Otherwise, :=, `:=`(...) and let(...) are defined for use in j, once only and in particular ways. See help(":=").

## clean up
rm(.datatable.aware)

I know there has been some nocov in the past, but the .datatable.aware flag might be nice.

@Nj221102
Copy link
Contributor Author

As for testing, I believe there is at least one existing test that appears to be somewhat indirect:

test(14.1, {example(':=', package='data.table', local=TRUE, echo=FALSE); TRUE})

Based on #5654 , we seem to have more options if we wanted to. This triggers an error when I have data.table loaded. We would want to clean up the flag if implemented in tests. Note, I am not on this branch so I get the old message.

.datatable.aware = TRUE
dt = data.table(a = 1L)
dt[, b:=2L]

## Error: Check that is.data.table(DT) == TRUE. Otherwise, :=, `:=`(...) and let(...) are defined for use in j, once only and in particular ways. See help(":=").

## clean up
rm(.datatable.aware)

I know there has been some nocov in the past, but the .datatable.aware flag might be nice.

done 👍 , thanks for helping :)

.datatable.aware = FALSE
dt = data.table(a = 1L)
test(2252.1, dt[, b:=2L], error = "^\\[ was called on a data.table in an environment that is not data.table-aware.*")
test(2252.2, dt[, b:=2L], error = "^\\[ was called on a data.table in an environment that is not data.table-aware.*")
Copy link
Contributor

Choose a reason for hiding this comment

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

The same test is duplicated AFAIU - maybe the second case should be let. Also, we should unset the .datatable.aware variable. Otherwise, once we add more tests which expect to be a data.table aware package, it will fail and cause some confusion. Another alternative is to evaluate it in a different environment context but I am not sure if that is necessary.

The easiest way to do it is just remove the variable (e.g., rm(.datatable.aware)).

Nj221102 and others added 2 commits March 25, 2024 08:26
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
@MichaelChirico
Copy link
Member

Thanks for the fix! Very helpful

@MichaelChirico MichaelChirico merged commit 566bff0 into master Mar 25, 2024
5 checks passed
@MichaelChirico MichaelChirico deleted the error branch March 25, 2024 23:56
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.

Improve := error for case when cedta() dispatches [.data.frame
3 participants