Skip to content

use ellipsis instead of ... in prettyprint#7766

Open
tomasy-j wants to merge 4 commits into
Rdatatable:masterfrom
tomasy-j:issue_7715_use_ellipsis_in_datatable_prettyprint
Open

use ellipsis instead of ... in prettyprint#7766
tomasy-j wants to merge 4 commits into
Rdatatable:masterfrom
tomasy-j:issue_7715_use_ellipsis_in_datatable_prettyprint

Conversation

@tomasy-j
Copy link
Copy Markdown

@tomasy-j tomasy-j commented May 28, 2026

Hello everyone!
I'm opening my 1st PR and would be happy to contribute to this amazing project going forward.
A little about me: I've been on and off data.table user for quite a few years now and consider myself rather proficient in R and I know some C as well, so hopefully I could contribute to some core data.table as well.

I introduced the small change described in issue #7715 and had to update some tests as well, namely:

1515.1, 2253.2, 2253.03, 2253.05, 2253.06, 2253.08, 2253.09,
2253.11, 2253.12, 2253.15, 2253.16, 2253.18, 2253.19

These 2 tests appear to be failing when I build the package locally (and in GH Actions jobs as well). I'll try to spend some time on it and fix, but any hints appreciated! @tdhock @ben-schwen @MichaelChirico

2253.2
2253.18

Thank you,
Tomasz

@tomasy-j tomasy-j requested a review from MichaelChirico as a code owner May 28, 2026 21:44
@tdhock tdhock self-requested a review May 29, 2026 07:08
@tdhock
Copy link
Copy Markdown
Member

tdhock commented May 29, 2026

Hi @tomasy-j thanks for contributing!
On CI I see

Test 2253.18 ran without errors but failed check that x equals y:
  > x = gsub(clean_regex, "", capture.output(print(DT))[-1L]) 
  First 3 of 3 (type 'character'): 
  [1] "こ    んん ááá"     "ここ  んんん ááá"   "こここ んんん… ááá"
  Non-ASCII string detected, raw representation:
  [[1]]
   [1] e3 81 93 20 20 20 20 e3 82 93 e3 82 93 20 61 cc 81 61 cc 81 61 cc 81
  
  [[2]]
   [1] e3 81 93 e3 81 93 20 20 e3 82 93 e3 82 93 e3 82 93 20 61 cc 81 61 cc 81 61
  [26] cc 81
  
  [[3]]
   [1] e3 81 93 e3 81 93 e3 81 93 20 e3 82 93 e3 82 93 e3 82 93 e2 80 a6 20 61 cc
  [26] 81 61 cc 81 61 cc 81
  
  > y = c(paste0(ja_ko, "      ", strrep(ja_n, 2L), " ", strrep(accented_a,      3L)), paste0(strrep(ja_ko, 2L), "    ", strrep(ja_n, 3L),      " ", strrep(accented_a, 3L)), paste(strrep(ja_ko, 3L), paste0(strrep(ja_n,      3L), dots), strrep(accented_a, 3L))) 
  First 3 of 3 (type 'character'): 
  [1] "こ      んん ááá"   "ここ    んんん ááá" "こここ んんん… ááá"
  Non-ASCII string detected, raw representation:
  [[1]]
   [1] e3 81 93 20 20 20 20 20 20 e3 82 93 e3 82 93 20 61 cc 81 61 cc 81 61 cc 81
  
  [[2]]
   [1] e3 81 93 e3 81 93 20 20 20 20 e3 82 93 e3 82 93 e3 82 93 20 61 cc 81 61 cc
  [26] 81 61 cc 81
  
  [[3]]
   [1] e3 81 93 e3 81 93 e3 81 93 20 e3 82 93 e3 82 93 e3 82 93 e2 80 a6 20 61 cc
  [26] 81 61 cc 81 61 cc 81
  
  2 string mismatches
  Test 2253.2 did not produce correct output:
  Expected: <<      a\n1: a...\n2: <NA>>>
  Observed: <<      a\n1:   a…\n2: <NA>>>

I think the expectations should be changed in these two tests.

You wrote that you "had to update some tests as well, namely: 1515.1, 2253.2, 2253.03, 2253.05, 2253.06, 2253.08, 2253.09, 2253.11, 2253.12, 2253.15, 2253.16, 2253.18, 2253.19" but did you forget to push? I only see changes to 1515.1, 2253.15, and 2253.16.

Copy link
Copy Markdown
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.

looks ok so far, please revise

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.85%. Comparing base (d4974e9) to head (f0ddd1a).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7766      +/-   ##
==========================================
- Coverage   99.04%   98.85%   -0.19%     
==========================================
  Files          87       87              
  Lines       17064    17122      +58     
==========================================
+ Hits        16901    16926      +25     
- Misses        163      196      +33     

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

@tomasy-j
Copy link
Copy Markdown
Author

Thank you @tdhock. Sorry, I wasn't precise - those tests were affected by the change, but didn't require tests' code modification. The 2 failing tests (2253.2, 2253.18) did need some changes though.

I'm not entirely sure about the 2 remaining checks that fail: codecov/project and atime performance tests / comment (pull request). Is the second one because data.table's CI cannot checkout my branch? WHenever you have a moment, do you mind letting me know how to proceed with these?

@tomasy-j tomasy-j marked this pull request as draft May 30, 2026 14:16
@tomasy-j tomasy-j marked this pull request as ready for review May 30, 2026 20:59
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.

2 participants