FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tryCatch for nchar() on old R by MichaelChirico · Pull Request #7848 · Rdatatable/data.table · GitHub

/ data.table Public

tryCatch for nchar() on old R - #7848

Merged
MichaelChirico merged 3 commits into
masterfrom
old-nchar-try
Jul 28, 2026
ConversationCommits (3)ChecksFiles changed
Merged

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

Conversation

MichaelChirico commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

See e.g.

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

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

codecov Bot commented Jul 27, 2026 •
edited
Loading

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 Bot commented Jul 27, 2026 •
edited
Loading

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

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

MichaelChirico mentioned this pull request Jul 27, 2026
Merged

joshhwuu commented Jul 27, 2026

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 commented Jul 27, 2026

Copy link
Copy Markdown
Member Author

Great spot! I find that on R 4.1.3:

https://github.com/wch/r-source/blob/7f6327b076889621a2e7244ef6c769f74c6012fb/src/main/character.c#L279

But not R 4.2.0:

https://github.com/wch/r-source/blob/775760668cadaf563fe771960c8a4cb1f5d8e75c/src/main/character.c#L279

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

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 commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

Great suggestion! Added the NEWS now

MichaelChirico changed the base branch from other-attach-order to master July 28, 2026 20:07
MichaelChirico merged commit d8f5c63 into master Jul 28, 2026
MichaelChirico deleted the old-nchar-try branch July 28, 2026 20:09
MichaelChirico mentioned this pull request Aug 25, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aitap aitap approved these changes
joshhwuu joshhwuu approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL