Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

export formatters functions #165

Closed
wants to merge 9 commits into from
Closed

export formatters functions #165

wants to merge 9 commits into from

Conversation

m7pr
Copy link
Contributor

@m7pr m7pr commented Jul 18, 2023

@m7pr m7pr marked this pull request as draft July 18, 2023 17:16
@m7pr m7pr added the core label Jul 18, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 18, 2023

badge

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ---------------------------------------------
R/basic_table_args.R            23       0  100.00%
R/draggable_buckets.R           82      82  0.00%    57-152
R/formatters_var_labels.R       51      51  0.00%    19-174
R/get_dt_rows.R                 13      13  0.00%    27-39
R/ggplot2_args.R                49       0  100.00%
R/include_css_js.R               7       1  85.71%   17
R/optionalInput.R              218     185  15.14%   87-362, 414, 449, 453, 457, 459, 465, 480-493
R/panel_group.R                 89      89  0.00%    12-130
R/plot_with_settings.R         382      21  94.50%   271-284, 343-344, 355-356, 610-611, 613, 615
R/standard_layout.R             40       0  100.00%
R/table_with_settings.R        173       1  99.42%   79
R/utils.R                        4       1  75.00%   7
R/verbatim_popup.R             100      49  51.00%   64-79, 105-106, 108, 116-144, 165
R/white_small_well.R             7       7  0.00%    18-24
TOTAL                         1238     500  59.61%

Diff against main

Filename                     Stmts    Miss  Cover
-------------------------  -------  ------  --------
R/formatters_var_labels.R      +51     +51  +100.00%
TOTAL                          +51     +51  -2.56%

Results for commit: cfc6a97

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Jul 18, 2023

Unit Tests Summary

    1 files    10 suites   19s ⏱️
  97 tests   97 ✔️ 0 💤 0
204 runs  204 ✔️ 0 💤 0

Results for commit c2541af.

♻️ This comment has been updated with latest results.

@m7pr m7pr requested a review from a team July 18, 2023 18:59
@m7pr m7pr marked this pull request as ready for review July 18, 2023 18:59
R/formatters_var_labels.R Outdated Show resolved Hide resolved
NAMESPACE Show resolved Hide resolved
@gogonzo
Copy link
Contributor

gogonzo commented Jul 19, 2023

Is it okey to have similar functions in different packages? In teal.data we have data_label (to add label attribute to data) and var_labels here.
What about shortening name from formatters_var_labels to varlabels?

#' x <- formatters_with_label(c(1, 2, 3), label = "Test")
#' obj_label(x)
formatters_with_label <- function(x, label) {
obj_label(x) <- label
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gogonzo do you think we also need to include obj_label from formatters
https://github.com/insightsengineering/formatters/blob/ffa09bb46eb28a9e269847b918c14c719c5d28b5/R/generics.R#L261C1-L283C2 ? Looks like it's a wrapper around S3 and S4 at the same time, where for S4 it uses standardGeneric and for S3 it just calls attr(x, "label"). I think in teal packages we focus on S3, so rewriting obj_label(x) <- label to attr(x, "label") <- label should be enough

@m7pr m7pr marked this pull request as draft July 20, 2023 07:31
@m7pr
Copy link
Contributor Author

m7pr commented Jul 28, 2023

@m7pr m7pr closed this Jul 28, 2023
@insights-engineering-bot insights-engineering-bot deleted the 447_remove_formatters@main branch October 22, 2023 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants