Add newline character support for ref footnotes #215

Melkiades · 2023-10-17T10:10:28Z

(pagination does not count it though for now) needed for insightsengineering/rtables#750

github-actions · 2023-10-17T10:14:00Z

Code Coverage Summary

Filename             Stmts    Miss  Cover    Missing
-----------------  -------  ------  -------  ---------------------------------------------------------------------------------------------------------------
R/format_value.R       197      14  92.89%   94, 110-117, 197, 216, 287, 409, 420, 428
R/generics.R            98       4  95.92%   455, 467, 500, 529
R/labels.R              55       7  87.27%   53, 59, 68, 109, 137, 146, 150
R/matrix_form.R        454      27  94.05%   400-401, 496, 508-509, 528, 559, 648-649, 664-669, 696-697, 729-730, 762-763, 795, 881, 929, 981, 983, 986
R/mpf_exporters.R      124      12  90.32%   2, 84-86, 180, 220, 225, 408, 410, 415-416, 442
R/page_size.R           45       1  97.78%   169
R/pagination.R         503      27  94.63%   421, 493, 613-614, 619, 673-674, 692-700, 986, 993, 1018-1025, 1160-1161, 1173-1174, 1186-1187
R/tostring.R           521      26  95.01%   83, 135, 202, 236, 244-245, 280, 333-334, 423-425, 432-435, 610-611, 803, 818, 913, 966, 1007, 1052, 1107, 1114
R/utils.R                3       0  100.00%
TOTAL                 2000     118  94.10%

Diff against main

Filename           Stmts    Miss  Cover
---------------  -------  ------  -------
R/matrix_form.R      +27       0  +0.38%
R/pagination.R        +5      +5  -0.95%
R/tostring.R         +22      +1  +0.02%
TOTAL                +54      +6  -0.14%

Results for commit: 8b5c380

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2023-10-17T10:14:17Z

Unit Tests Summary

    1 files     4 suites 8s ⏱️
  33 tests   33 ✔️ 0 💤 0 ❌
228 runs 228 ✔️ 0 💤 0 ❌

Results for commit 8b5c380.

♻️ This comment has been updated with latest results.

…htsengineering/formatters into 750_solve_nl_in_export_as_txt@main

Melkiades · 2023-10-17T12:09:54Z

R/pagination.R

+        page_indices$pag_row_indices,
+        function(ii) {
+          mf_tmp <- matrix_form(obj[ii, ], TRUE, TRUE, indent_size = indent_size)
+          mf_col_widths(mf_tmp) <- colwidths


@shajoezhu very important fix that was avoiding the wrapping when paginated

Melkiades · 2023-10-17T22:17:10Z

R/tostring.R

+      if (!setequal(ori_wrapped_txt_v, cur_wrapped_txt_v)) {
+        return(wrap_string(str = ret_collapse, width = width, collapse = collapse))


This avoids infinite loop

Melkiades · 2023-10-17T22:18:42Z

R/matrix_form.R

+  # pre-proc in case of wrapping and \n
+  line_grouping <- mf_lgrouping(matform)
+  strmat <- .compress_mat(strmat, line_grouping, "nl")
+  frmmat <- .compress_mat(frmmat, line_grouping, "unique") # never not unique
+  spamat <- .compress_mat(spamat, line_grouping, "unique")
+  alimat <- .compress_mat(alimat, line_grouping, "unique")
+  line_grouping <- unique(line_grouping)


this compression is needed when columns are wrapped. Without it we get a weird bottom alignment if there are \n preceding this

Melkiades · 2023-10-17T22:19:30Z

R/matrix_form.R

+    tl <- strmat[nl_inds_header, 1, drop = TRUE]
+    strmat[nl_inds_header, 1] <- ""


This fix is needed when the number of rows are not the number of lines of the header (we need to get topleft out and add it back later to respect the right line grouping)

Melkiades · 2023-10-17T22:20:51Z

R/matrix_form.R

+        val <- unique(col_vec)
+        val <- val[nzchar(val)]
+        if (length(val) > 1) {
+          stop("Problem in linegroupings! Some do not have the same values.") # nocov
+        } else if(length(val) < 1) {
+          val <- "" # Case in which it is only ""
+        }
+        val[[1]]


These are done for formats (that can be not unique, e.g. c("1", "") and val[[1]] is needed when there are functions (automatic cast to lists)

Melkiades · 2023-10-17T22:22:22Z

R/tostring.R

+    # Fix for ref_fnotes with \n characters XXX this does not count in the pagination
+    if (any(grepl("\n", ref_fnotes))) {
+      ref_fnotes <- unlist(strsplit(ref_fnotes, "\n", fixed = TRUE))
+    }
+


this is a safenet, being it in toString, it will not update the number of lines for pagination but neither break (I do not expect this to be used at all in general)

Melkiades · 2023-10-17T22:22:56Z

R/tostring.R

@@ -678,11 +684,6 @@ new_line_warning <- function(str_v) {
 #' @param collapse character(1) or `NULL`. If the words that have been split should
 #'   be pasted together with the collapse character. This is usually done internally
 #'   with `"\n"` to have the wrapping updated along with other internal values.
-#' @param smart logical(1). Defaults to `TRUE`. It attempts to calculate the optimal


we use only the "smart" recursive algorithm

Melkiades · 2023-10-17T22:23:17Z

R/tostring.R

@@ -778,14 +779,40 @@ wrap_string <- function(str, width, collapse = NULL, smart = TRUE) {
  return(ret)
 }

+.go_stri_wrap <- function(str, w) {


compacted it

Melkiades · 2023-10-17T22:25:55Z

R/tostring.R

+        ret_tmp <- force_split_words_by(ret[we_interval], width) # here we_interval is only one ind
+        ret <- append(ret, ret_tmp, we_interval)[-we_interval]
+        which_exceeded <- which(nchar(ret) > width)


force_split was necessary because stringi appears unable to split certain characters such as "x ." into smaller elements. I suspect that space and full stop are considered one word (even if normalize = TRUE) so it gets stuck in an eternal recursion, so we need to break it manually, append it and redefine the index of what is needed to be modified

Melkiades · 2023-10-17T22:27:38Z

R/tostring.R

+      init_v <- seq(1, nchar(wrd_i), by = width)
+      end_v <- c(init_v[-1] - 1, nchar(wrd_i))
+      str_v_tmp <- stringi::stri_sub(wrd_i, from = init_v, to = end_v)
+      ret_tmp <- c(ret_tmp, str_v_tmp[!grepl("^\\s+$", str_v_tmp) & nzchar(str_v_tmp)])


avoiding empties is needed at this stage to avoid spurious spaces appearing (it should not happen too often to get here anyway, only with seriously small widths)

Melkiades · 2023-10-17T22:27:56Z

tests/testthat/test-exporters.R

+        expct_lns <- c("              lo     ",
+                       "  hi        there    ",
                       "(N=50)   (N=whoknows)",


I value this as an improvement

Melkiades · 2023-10-17T22:28:55Z

tests/testthat/test-exporters.R

+                                has_topleft = FALSE,
                                line_grouping = c(1, 1, 2, 3, 4),


not having to add topleft info was a problem as it was mistakingly considering the first colname as a topleft info

Melkiades · 2023-10-17T22:29:23Z

vignettes/formatters.Rmd

@@ -135,7 +135,7 @@ matrix_form.data.frame <- function(df) {
    strings = strings,
    aligns = aligns,
    spans = matrix(1, nrow = fnr, ncol = fnc),
-    formats = NULL,
+    formats = matrix("", nrow = fnr, ncol = fnc),


This should not be allowed in general

shajoezhu

thanks a lot @Melkiades , i am merging this in to trigger UAT

Melkiades added 11 commits October 13, 2023 17:21

it seems to work already

f23b9d8

Fix + News

6cc5a70

fantastic

3ab6d50

fix

ca83046

VERY IMPORTANT FIX

76bfada

fix topleft alignment as bottom left

c509735

fix topleft alignment as bottom left

49642b4

multiple fixes for bottom topleft

caf35a1

fix

999ec56

conflit solved

e322670

fix for ref_fnotes

cdb6bbc

Melkiades added bug Something isn't working sme Tracks changes for the sme board labels Oct 17, 2023

Melkiades requested review from edelarua, shajoezhu and insights-engineering-bot as code owners October 17, 2023 10:10

Merge branch 'main' into 750_solve_nl_in_export_as_txt@main

d31eb5b

Melkiades added 2 commits October 17, 2023 14:05

fix for infinite stack

1140258

Merge branch '750_solve_nl_in_export_as_txt@main' of github.com:insig…

5e331d6

…htsengineering/formatters into 750_solve_nl_in_export_as_txt@main

Melkiades commented Oct 17, 2023

View reviewed changes

Melkiades and others added 8 commits October 17, 2023 14:10

Fix

7f3b806

refixes

efc31fa

correct fix of the loop

b0f205a

[skip actions] Roxygen Man Pages Auto Update

e50733e

finally fixed

5c1e4c1

empty

1a06eec

no topleft!

15ed76a

may be a fix

fbd09cf

Melkiades and others added 2 commits October 17, 2023 18:47

final fix

8f674b2

Merge branch 'main' into 750_solve_nl_in_export_as_txt@main

8b5c380

Melkiades commented Oct 17, 2023

View reviewed changes

Merge branch 'main' into 750_solve_nl_in_export_as_txt@main

52eb394

shajoezhu enabled auto-merge (squash) October 18, 2023 03:42

shajoezhu approved these changes Oct 18, 2023

View reviewed changes

shajoezhu merged commit 42dceb4 into main Oct 18, 2023
14 of 18 checks passed

shajoezhu deleted the 750_solve_nl_in_export_as_txt@main branch October 18, 2023 03:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add newline character support for ref footnotes #215

Add newline character support for ref footnotes #215

Melkiades commented Oct 17, 2023

github-actions bot commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

Melkiades Oct 17, 2023

shajoezhu left a comment

		if (!setequal(ori_wrapped_txt_v, cur_wrapped_txt_v)) {
		return(wrap_string(str = ret_collapse, width = width, collapse = collapse))

		tl <- strmat[nl_inds_header, 1, drop = TRUE]
		strmat[nl_inds_header, 1] <- ""

Add newline character support for ref footnotes #215

Add newline character support for ref footnotes #215

Conversation

Melkiades commented Oct 17, 2023

github-actions bot commented Oct 17, 2023 • edited Loading

Code Coverage Summary

Diff against main

github-actions bot commented Oct 17, 2023 • edited Loading

Unit Tests Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shajoezhu left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading