fix package --as-cran

[donyunardi]

related to https://github.com/insightsengineering/coredev-tasks/issues/334

[donyunardi]

Some of the examples are over 5 seconds to run.

We could make synthetic_cdisc_data only get called once:

synthetic_cdisc_data_latest <- synthetic_cdisc_data("latest")
  ADSL <- synthetic_cdisc_data_latest$adsl
  ADAE <- synthetic_cdisc_data_latest$adae
  ADCM <- synthetic_cdisc_data_latest$adcm

If so, can I keep the code argument as is?

      cdisc_dataset("ADSL", ADSL, code = 'ADSL <- synthetic_cdisc_data("latest")$adsl'),
      cdisc_dataset("ADAE", ADAE, code = 'ADAE <- synthetic_cdisc_data("latest")$adae'),

@nikolas-burkoff @mhallal1
Let me know your thought.

[donyunardi]

Hi @arkadiuszbeer
There's a linting error during the check workflow:

Warning: file=/github/workspace/R/tm_g_lineplot.R,line=221,col=2,[commented_code_linter] Commented code should be removed.

Warning: file=/github/workspace/R/tm_t_summary_by.R,line=341,col=2,[commented_code_linter] Commented code should be removed.

Warning: file=/github/workspace/R/utils.R,line=409,col=15,[vector_logic_linter] Conditional expressions require scalar logical operators (&& and ||)
Warning: file=/github/workspace/R/utils.R,line=420,col=22,[vector_logic_linter] Conditional expressions require scalar logical operators (&& and ||)

I'm not sure what to do about the commented code. Both lines look okay to me.
And for scalar logical operator, isn't in R we can do logical operator using & and &&?
Are we enforcing the usage of &&?

[mhallal1]

Some of the examples are over 5 seconds to run.

We could make synthetic_cdisc_data only get called once:

synthetic_cdisc_data_latest <- synthetic_cdisc_data("latest")
  ADSL <- synthetic_cdisc_data_latest$adsl
  ADAE <- synthetic_cdisc_data_latest$adae
  ADCM <- synthetic_cdisc_data_latest$adcm

If so, can I keep the code argument as is?

      cdisc_dataset("ADSL", ADSL, code = 'ADSL <- synthetic_cdisc_data("latest")$adsl'),
      cdisc_dataset("ADAE", ADAE, code = 'ADAE <- synthetic_cdisc_data("latest")$adae'),

@nikolas-burkoff @mhallal1 Let me know your thought.

@donyunardi Yeah i think that you keep the code argument as technically it is the same code but we are pulling it once instead of twice before launching the app.

[arkadiuszbeer]

hi @donyunardi

Hi @arkadiuszbeer There's a linting error during the check workflow:

Warning: file=/github/workspace/R/tm_g_lineplot.R,line=221,col=2,[commented_code_linter] Commented code should be removed.

Warning: file=/github/workspace/R/tm_t_summary_by.R,line=341,col=2,[commented_code_linter] Commented code should be removed.

Examples in files tm_g_lineplot.R,:221 and tm_t_summary_by.R:341 should not have ' (single quotation mark), becuase linter will treat it as end of comment.

Warning: file=/github/workspace/R/utils.R,line=409,col=15,[vector_logic_linter] Conditional expressions require scalar logical operators (&& and ||)
Warning: file=/github/workspace/R/utils.R,line=420,col=22,[vector_logic_linter] Conditional expressions require scalar logical operators (&& and ||)

I'm not sure what to do about the commented code. Both lines look okay to me. And for scalar logical operator, isn't in R we can do logical operator using `&` and `&&`? Are we enforcing the usage of `&&`?

No. For bit operations and vector operation we could use &, but for logical operation we should use &&

[donyunardi]

Thanks @mhallal1 and @Polkas for the suggestion.

It looks good to me locally.

@arkadiuszbeer
I'm going to open a separate issue for the logical operator issue during lint check process.

[mhallal1]

Additionally, I got this note:

 Imports includes 26 non-default packages.
  Importing from so many packages makes the package vulnerable to any of
  them becoming unavailable.  Move as many as possible to Suggests and
  use conditionally.

[github-actions]

Unit Tests Summary

    1 files    32 suites   12s ⏱️
140 tests 140 ✔️ 0 💤 0 ❌
155 runs  155 ✔️ 0 💤 0 ❌

Results for commit 8f6cae8.

♻️ This comment has been updated with latest results.

[mhallal1]

Failing tests:

[mhallal1]

So below is the time needed per example on main, given this I dont think we should set check = FALSE to all examples as it is important to check if these examples work or not. I suggest to set check = FALSE only where needed.

EDIT: I can see that the different in time needed is remarkable between my machine and yours @donyunardi , the benchmark should be whatever CRAN is using which we can not test for now as our packages are not on CRAN yet :(

[donyunardi]

EDIT: I can see that the different in time needed is remarkable between my machine and yours @donyunardi , the benchmark should be whatever CRAN is using which we can not test for now as our packages are not on CRAN yet :(

True. So in this case, I only removed the check argument for the examples that were mentioned in my initial comments.
Hopefully that's good for now.

[donyunardi]

Additionally, I got this note:

 Imports includes 26 non-default packages.
  Importing from so many packages makes the package vulnerable to any of
  them becoming unavailable.  Move as many as possible to Suggests and
  use conditionally.

There's nothing we can do about this for now.
Everything else is addressed.

[mhallal1]

Let us create an issue for this as cran will shout out because of the number of imports (similar to insightsengineering/teal.modules.general#456)

[mhallal1]

LGTM.
Let us create an issue for the number of imports as cran will complain about it (similar to insightsengineering/teal.modules.general#456)