Custom formatting function and handling of na_str when using a formatting function #280

iaugusty · 2024-04-29T11:35:07Z

Please find some proposed extra features for formatters package.

main features of custom formatting function:

xx notation not restricted to predefined ones
For 2 and 3-d stats, the precision of each stat does not have to be the same to all, eg xx.x (xx.xx) for mean (sd)
flexible string notation, not restricted to predefined strings

format_value: also allow na_str to be passed onto a custom formatting function when na_str is a parameter of the function

We are open to other proposals for the naming of the proposed custom functions.
Also open to discuss anything else included in this PR.

…ed in jjcsformats

github-actions · 2024-04-29T11:35:20Z

CLA Assistant Lite bot ✅ All contributors have signed the CLA

iaugusty · 2024-04-29T11:39:24Z

I have read the CLA Document and I hereby sign the CLA

Melkiades · 2024-05-13T13:43:07Z

DESCRIPTION

@@ -35,6 +36,8 @@ Suggests:
    r2rtf (>= 0.3.2),
    rmarkdown (>= 2.19),
    stringi (>= 1.6),
+    stringr (>= 1.5.0),
+    tern (>= 0.9.3),


This is a circular dependency we need to avoid also from suggests

Melkiades · 2024-05-13T13:45:06Z

DESCRIPTION

@@ -35,6 +36,8 @@ Suggests:
    r2rtf (>= 0.3.2),
    rmarkdown (>= 2.19),
    stringi (>= 1.6),
+    stringr (>= 1.5.0),


We did a lot of work to avoid importing {stringr} and having only {stringi} in suggests so to have a lighter dep tree as this package is the basis of much of what we do. We have already implemented some ways to read in any xx input, we just lacked the time to add it to {formatters}

Melkiades · 2024-05-13T13:46:01Z

DESCRIPTION

@@ -25,7 +25,8 @@ Depends:
 Imports:
    checkmate (>= 2.1.0),
    grid,
-    htmltools (>= 0.5.3)
+    htmltools (>= 0.5.3),
+    tidytlg (>= 0.1.4)


imports in this package should be very limited. Where is it needed? we usually add {pkg}:: everywhere we use external packages in all our packages

Melkiades · 2024-05-13T13:46:29Z

NAMESPACE

@@ -153,6 +157,7 @@ importFrom(grid,unit)
 importFrom(htmltools,tagList)
 importFrom(htmltools,tags)
 importFrom(stats,na.omit)
+importFrom(tidytlg,roundSAS)


isnt it possible to copy? it is probably not worth the import

Melkiades · 2024-05-13T13:47:00Z

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: formatters
 Title: ASCII Formatting for Values and Tables
-Version: 0.5.6.9002
+Version: 0.5.6.9003


this will happen automatically once we merge the PR ;) could you revert this?

Melkiades · 2024-05-13T13:58:26Z

R/jjcsformats.R

+#' @examples
+#' jjcsformat_xx_SAS <- jjcs_format_xx_fct(roundmethod="SAS")
+#' jjcsformat_xx <- jjcsformat_xx_SAS
+#' format_value(c(1.453),jjcsformat_xx("xx.xx"))


Suggested change

#' format_value(c(1.453),jjcsformat_xx("xx.xx"))

#' format_value(c(1.453), jjcsformat_xx("xx.xx"))

Please use a styler to fix these issues (they will not pass the tests, too, I think)

Melkiades · 2024-05-13T14:05:59Z

R/jjcsformats.R

+#' format_value(c(1.453),jjcsformat_xx("xx.xx"))
+#' format_value(c(1.453,2.45638),jjcsformat_xx("xx.xx (xx.xxx)"))
+#'
+jjcs_format_xx_fct <- function (roundmethod=c("SAS","R"))


Suggested change

jjcs_format_xx_fct <- function (roundmethod=c("SAS","R"))

jjcs_format_xx_fct <- function(round_method = c("SAS", "R"))

We use snake case fore all our variables and fnc. Camel case ONLY for Classes

Melkiades · 2024-05-13T14:07:24Z

R/jjcsformats.R

+
+  fnct <- function(str){
+
+    if (stringr::str_detect(str,stringr::fixed("xxx."))) {


use stringi or base R instead of stringr. I know it seems tedious but the stringr footprint is much larger

this will be the most tricky one, I'll look into this to use stringi instead

Melkiades · 2024-05-13T14:07:48Z

R/jjcsformats.R

+        format(roundfunc(x, digits = ifelse(length(y) >
+                                              1, nchar(y[2]), 0)),nsmall = ifelse(length(y) >
+                                                                                    1, nchar(y[2]), 0))
+      }


Use styler for correct spacing

Melkiades · 2024-05-13T14:09:38Z

R/jjcsformats.R

+      return("-")
+    }
+
+    tern:::assert_proportion_value(fraction, include_boundaries = TRUE)


do not use internals of other packages in general. In this case we absolutely want to avoid circular dependencies

Melkiades · 2024-05-13T14:13:02Z

tests/testthat/test-jjcsformats.R

+
+
+
+  # expect_identical(


commented code is not allowed in testthat

Melkiades · 2024-05-13T14:15:12Z

R/jjcsformats.R

+#'   roundSAS comes from this Stack Overflow post https://stackoverflow.com/questions/12688717/round-up-from-5
+#'   * `R`: the underlying rounding method is `round`
+#'
+#' @return `jjcs_format_xx_fct()` format function that can be used in rtables formatting calls


why jjcs? I think these functions may well be of general use, so probably better to have a user-friendly name for these format functions?

jjcs was coming from our internal project, I agree that we should come up with a better name for this, if you would have any suggestions, these would be more than welcome

Merge branch 'main' of https://github.com/iaugusty/formatters # Conflicts: # DESCRIPTION

iaugusty · 2024-05-28T12:50:25Z

@Melkiades I reforked the repository to get the latest version of formatters, how do I re-open the current PR?

iaugusty · 2024-05-28T12:50:42Z

x

iaugusty · 2024-05-28T12:52:05Z

@Melkiades Shall I just start a new PR, or shall I wait until the truetype font PR is completed?

Melkiades · 2024-05-28T12:56:37Z

, how do I re-open the current PR?

I think it is better to solve all the issue on this PR (even if you want to create a new one) so I can keep track of changes. When you need to update your local clone branch with main it is enough you git pull and main from the original repo should be tracked so no need to close and reopen

iaugusty · 2024-05-28T13:03:58Z

As I had deleted my fork, the pull request was closed.
I have reforked, and commited my changes. However, as the pull request was closed, the below "reopen and comment" button states "the repository that submitted this pull request has been deleted", it doesn't recognize my new reforked version.
Do you know an alternative method in github where I can reopen the PR?

Melkiades · 2024-05-28T13:50:55Z

As I had deleted my fork, the pull request was closed. I have reforked, and commited my changes. However, as the pull request was closed, the below "reopen and comment" button states "the repository that submitted this pull request has been deleted", it doesn't recognize my new reforked version. Do you know an alternative method in github where I can reopen the PR?

I do not know! Try to address my comments and I will review again. I would wait, though for resubmission as soonish you should have rights to branch ;)

fyi @cicdguy @shajoezhu

cicdguy · 2024-05-28T14:05:34Z

@iaugusty and @gmbecker - you both should be receiving 5 invitations in the mailbox that is associated with your GitHub accounts. One each for tern, formatters, rtables, rlistings, and scda.test. On accepting these invitations, you should have write access to these repos, so you'll no longer need to fork going forward.

iaugusty added 5 commits April 15, 2024 11:53

na_str for formatting functions

52b32fa

minor update

9240890

function factory for count/denom/fraction formatting function

2fc1049

finetuning and add test scripts for custom formatting functions defin…

e6c41ff

…ed in jjcsformats

reset to roxygen 7.3.1

0783c53

iaugusty requested review from edelarua, Melkiades and ayogasekaram as code owners April 29, 2024 11:35

iaugusty and others added 2 commits April 29, 2024 17:49

Merge branch 'main' into main

af26d56

[skip actions] Bump version to 0.5.6.9003

5e49a6e

Melkiades reviewed May 13, 2024

View reviewed changes

tests/testthat/test-jjcsformats.R Outdated

# expect_identical(

Copy link

Contributor

Melkiades May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented code is not allowed in testthat

Melkiades reviewed May 13, 2024

View reviewed changes

iaugusty and others added 3 commits May 28, 2024 12:02

updates per review comments

4d54041

rebase

cb8b4b5

Merge branch 'main' of https://github.com/iaugusty/formatters # Conflicts: # DESCRIPTION

[skip actions] Bump version to 0.5.6.9004

d8d50c0

iaugusty closed this by deleting the head repository May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom formatting function and handling of na_str when using a formatting function #280

Custom formatting function and handling of na_str when using a formatting function #280

iaugusty commented Apr 29, 2024

github-actions bot commented Apr 29, 2024 •

edited

Loading

iaugusty commented Apr 29, 2024

Melkiades May 13, 2024

Melkiades May 13, 2024

Melkiades May 13, 2024

Melkiades May 13, 2024

Melkiades May 13, 2024

Melkiades May 13, 2024

Melkiades May 13, 2024

Melkiades May 13, 2024

iaugusty May 13, 2024

Melkiades May 13, 2024

Melkiades May 13, 2024

Melkiades May 13, 2024

Melkiades May 13, 2024

iaugusty May 13, 2024

iaugusty commented May 28, 2024

iaugusty commented May 28, 2024

iaugusty commented May 28, 2024

Melkiades commented May 28, 2024

iaugusty commented May 28, 2024

Melkiades commented May 28, 2024

cicdguy commented May 28, 2024

	#' format_value(c(1.453),jjcsformat_xx("xx.xx"))
	#' format_value(c(1.453), jjcsformat_xx("xx.xx"))

	jjcs_format_xx_fct <- function (roundmethod=c("SAS","R"))
	jjcs_format_xx_fct <- function(round_method = c("SAS", "R"))


		fnct <- function(str){

		if (stringr::str_detect(str,stringr::fixed("xxx."))) {

Custom formatting function and handling of na_str when using a formatting function #280

Custom formatting function and handling of na_str when using a formatting function #280

Conversation

iaugusty commented Apr 29, 2024

github-actions bot commented Apr 29, 2024 • edited Loading

iaugusty commented Apr 29, 2024

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

Choose a reason for hiding this comment

iaugusty commented May 28, 2024

iaugusty commented May 28, 2024

iaugusty commented May 28, 2024

Melkiades commented May 28, 2024

iaugusty commented May 28, 2024

Melkiades commented May 28, 2024

cicdguy commented May 28, 2024

github-actions bot commented Apr 29, 2024 •

edited

Loading