add escape for datatable unaware for package

[ben-schwen]

Closes #2422

But do not know how we should properly test this?

[dvg-p4]

#5655 might help with testing :-)

[MichaelChirico]

I actually think this fix is pretty reasonable, WDYT @jangorecki?

[MichaelChirico]

@ben-schwen I'll let you merge since this was marked as Draft but it's good to go IMO

[github-actions]

• HEAD=dim_cedta stopped early for DT[by,verbose=TRUE] improved in #6296
• HEAD=dim_cedta slower P<0.001 for memrecycle regression fixed in #5463
• HEAD=dim_cedta slower P<0.001 for setDT improved in #5427
  

Generated via commit 1899eab

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

Task	Duration
R setup and installing dependencies	2 minutes and 53 seconds
Installing different package versions	41 seconds
Running and plotting the test cases	2 minutes and 34 seconds

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.77%. Comparing base (8f5ffa8) to head (1899eab).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5918   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files          81       81           
  Lines       15203    15203           
=======================================
  Hits        15017    15017           
  Misses        186      186           

☔ 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.

[MichaelChirico]

Hm, could you explain these? Needing it is a bit of a code smell...

[ben-schwen]

So, basically when deactivating cedta for dim we are facing the problem that nrow dispatches to dim(x)[1L]. Now if !cedta we have a problem for objects which are data.table but no data.frame and have no dim method because then it can happen that nrow returns NULL which bites us for example at print.data.table.

p.s. I have no idea why vscode keeps adding the french datatable-joins vignette (thats also the reason for the force-push) but I suppose it has something to do with line endings.

[jangorecki]

What's the exact reason to forcing verbose to F here? Why in this particular method we want to see less?

[ben-schwen]

The reason is that we call dim in every print statement which are often not cedta

[MichaelChirico]

Isn't that resolved by switching to nrow(x)[1L] in print.data.table?

We could instead call .Call(Cdim, x)[1L] right?

---

I do see other dim() usage in print.data.table:

data.table/R/print.data.table.R

Line 55 in 8f5ffa8

if (any(dim(x)==0L)) {

So maybe we want dim_x = .Call(Cdim, x) & proceed from there?

[jangorecki]

Yes, please revert changes to vignettes/fr/datatable-joins.Rmd
Later on when doing git add just list files to add so only specified files will get into commit.

[MichaelChirico]

p.s. I have no idea why vscode keeps adding the french datatable-joins vignette (thats also the reason for the force-push) but I suppose it has something to do with line endings.

I was getting the same thing leading to #7184, I guess if you revert and merge master it will stop.

My understanding is we have some setting to auto-convert CRLF to LF, which shows up as a "ghost" editing the CRLF file & impossible to get around.

Edit: I'm pretty sure it's our .gitattributes:

data.table/.gitattributes

Line 1 in 8f5ffa8

* text eol=lf

I'm not sure why that wasn't applied within GitHub.

[MichaelChirico]

we have a problem for objects which are data.table but no data.frame and have no dim method

Hm, on the one hand such objects are kind of broken already, but I suppose they're found here and there. Maybe

if (!cedta() && !is.null(df_dim <- NextMethod())) return(df_dim)
.Call(Cdim, x)

[aitap]

That's because we have `* text eol=lf` in .gitattributes. You can try temporarily creating but not committing vignettes/.gitattributes with the contents `* -text` to tell Git to get out of your way for now.

[ChristianWia]

Yes, please revert changes to vignettes/fr/datatable-joins.Rmd Later on when doing git add just list files to add so only specified files will get into commit.

please dont revert the work, several days have been spent to complete the task. Let us try on another product. Thanks.

[MichaelChirico]

@ChristianWia thank you for your concern :) To be clear, the aim is only to revert the changes in this PR, which are spurious. The file in master would remain as-is.