Skip to content

tryCatch for nchar() on old R - #7848

Merged
MichaelChirico merged 3 commits into
masterfrom
old-nchar-try
Jul 28, 2026
Merged

tryCatch for nchar() on old R#7848
MichaelChirico merged 3 commits into
masterfrom
old-nchar-try

Conversation

@MichaelChirico

@MichaelChirico MichaelChirico commented Jul 27, 2026

Copy link
Copy Markdown
Member

See e.g.

h/t @joshhwuu for identifying the culprit R bugfix in the comment below.

@MichaelChirico
MichaelChirico requested a review from aitap July 27, 2026 07:13
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.01%. Comparing base (f1edc68) to head (a0baec7).

Additional details and impacted files
@@                 Coverage Diff                 @@
##           other-attach-order    #7848   +/-   ##
===================================================
  Coverage               99.01%   99.01%           
===================================================
  Files                      88       88           
  Lines                   17286    17286           
===================================================
  Hits                    17116    17116           
  Misses                    170      170           

☔ View full report in Codecov by Harness.
📢 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.

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
  • HEAD=old-nchar-try much slower for transform improved in #5493
    Comparison Plot

Generated via commit a0baec7

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

Task Duration
R setup and installing dependencies 2 minutes and 48 seconds
Installing different package versions 1 minutes and 9 seconds
Running and plotting the test cases 5 minutes and 35 seconds

@joshhwuu

Copy link
Copy Markdown
Member

changes look good to me..

this may be the fix on R's side: r-devel/r-svn@a786d1e

@MichaelChirico

Copy link
Copy Markdown
Member Author

@MichaelChirico
MichaelChirico force-pushed the old-nchar-try branch 2 times, most recently from 401eb10 to c4a4fee Compare July 28, 2026 05:37
@aitap

aitap commented Jul 28, 2026

Copy link
Copy Markdown
Member

Since nchar(allowNA = TRUE) mistakenly signals an error instead of returning NA, we could also use

diff --git a/R/print.data.table.R b/R/print.data.table.R
index e602f80d6..817e49420 100644
--- a/R/print.data.table.R
+++ b/R/print.data.table.R
@@ -256,7 +256,7 @@ char.trunc = function(x, trunc.char = getOption("datatable.prettyprint.char")) {
   if (is.null(trunc.char)) return(x)
   trunc.char = max(0L, suppressWarnings(as.integer(trunc.char[1L])), na.rm=TRUE)
   if (!is.character(x) || trunc.char <= 0L) return(x)
-  nchar_width = nchar(x, 'width', allowNA = TRUE)
+  nchar_width = tryCatch(nchar(x, 'width', allowNA = TRUE), error = function(...) NA)
   nchar_chars = nchar(x, 'char', allowNA = TRUE)
   is_full_width = nchar_width > nchar_chars
   is_full_width[is.na(is_full_width)] = FALSE

and let the rest of the code handle the NA like it currently does on R ≥ 4.2.

@MichaelChirico

Copy link
Copy Markdown
Member Author

Great suggestion! Added the NEWS now

@MichaelChirico
MichaelChirico changed the base branch from other-attach-order to master July 28, 2026 20:07
@MichaelChirico
MichaelChirico merged commit d8f5c63 into master Jul 28, 2026
@MichaelChirico
MichaelChirico deleted the old-nchar-try branch July 28, 2026 20:09
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.

3 participants