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

... (ellipsis) not used in t_summarytable #98

Open
denisovan31415 opened this issue Jan 24, 2022 · 5 comments
Open

... (ellipsis) not used in t_summarytable #98

denisovan31415 opened this issue Jan 24, 2022 · 5 comments
Labels
bug Something isn't working core good first issue Good for newcomers

Comments

@denisovan31415
Copy link
Contributor

t_summarytable <- function(data,
                           trt_group,
                           param_var,
                           param,
                           xaxis_var,
                           facet_var = "AVISITCD",
                           loq_flag_var = "LOQFL", 
                           ...) {

The ... is not used anyway. Should be removed along with the documentation

@denisovan31415 denisovan31415 added bug Something isn't working core labels Jan 24, 2022
@nikolas-burkoff nikolas-burkoff self-assigned this Jan 25, 2022
@nikolas-burkoff nikolas-burkoff linked a pull request Jan 25, 2022 that will close this issue
@nikolas-burkoff
Copy link
Contributor

Not that simply removing the "..." which actually cause errors in teal.goshawk (🤯 ) here

After discussion with @gogonzo moving this to the backlog to handle after UAT.

@nikolas-burkoff nikolas-burkoff removed their assignment Jan 25, 2022
@donyunardi
Copy link
Contributor

donyunardi commented Oct 26, 2023

There are two modules in tgoshawk that are using this function:

https://github.com/insightsengineering/teal.goshawk/blob/348f38eaecb68b170e3e47e583a3d6cae45cc566/R/tm_g_gh_density_distribution_plot.R#L398

https://github.com/insightsengineering/teal.goshawk/blob/348f38eaecb68b170e3e47e583a3d6cae45cc566/R/tm_g_gh_boxplot.R#L515

At first, I thought ... is used for tm_g_gh_density_distribution_plot to capture the font-size argument, but looking at code base here, it doesn't look like the value passed in ... are being executed in the function's body. I don't even see ... was captured into a list.

which means, this line has no meaning to tgoshawk and has been working for because of the ... in goshawk.

So, if we remove the ... from t_summarytable, then we have to follow it by removing this line.

Just to be safe, pinging @npaszty in case there's other concern.

@npaszty
Copy link
Contributor

npaszty commented Oct 26, 2023

yes. t_summary used to present the tables below the visualizations

@donyunardi
Copy link
Contributor

Thanks @npaszty.

In that case, we'll move forward with the removal of ellipsis from t_summarytable since there's no use of it.

Acceptance Criteria:

  1. Remove ... argument from the t_summarytable function in goshawk
  2. Remove the font-size argument in teal.goshawk::srv_g_density_distribution_plot
  3. Pass R CMD check and integration test.

These two has to be done (merged) at the same time otherwise the integration test will fail.

@donyunardi donyunardi added the good first issue Good for newcomers label Oct 26, 2023
@npaszty
Copy link
Contributor

npaszty commented Oct 26, 2023

yeah, I guess I can't provide much more info than that since I haven't looked at this code for a very long time since its development/maintenance was taken over by the IE team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants