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

update staged.dependencies #224

Merged
merged 6 commits into from
Jul 20, 2022
Merged

update staged.dependencies #224

merged 6 commits into from
Jul 20, 2022

Conversation

BFalquet
Copy link
Contributor

add nestcolor as upstream dependencies

thank you for the review

@BFalquet BFalquet requested a review from edelarua July 20, 2022 12:02
@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2022

Code Coverage Summary

Filename                           Stmts    Miss  Cover    Missing
-------------------------------  -------  ------  -------  ---------
R/aet01.R                            312     312  0.00%    39-584
R/aet02.R                            197     197  0.00%    29-479
R/aet03.R                             66      66  0.00%    37-153
R/aet04.R                             86      86  0.00%    48-181
R/assertions.R                        42      42  0.00%    21-104
R/checks.R                            14      14  0.00%    15-48
R/chevron_tlg-S4class.R               19      19  0.00%    37-90
R/chevron_tlg-S4methods.R             14      14  0.00%    15-117
R/cmt01a.R                           156     156  0.00%    46-433
R/cmt02_pt.R                          37      37  0.00%    34-108
R/dmt01.R                             29      29  0.00%    44-125
R/dst01.R                            231     231  0.00%    5-619
R/dtht01.R                            77      77  0.00%    42-190
R/egt01.R                             44      44  0.00%    44-134
R/egt02.R                             56      56  0.00%    38-222
R/ext01.R                             69      69  0.00%    38-254
R/gen_args.R                           1       1  0.00%    19
R/lbt01.R                             39      39  0.00%    41-123
R/mht01.R                             57      57  0.00%    38-138
R/sample_study_object.R               14      14  0.00%    14-35
R/standard_data_preprocessing.R      240     240  0.00%    52-531
R/utils.R                            125     125  0.00%    11-274
R/vst01.R                             45      45  0.00%    44-136
R/vst02.R                             92      92  0.00%    39-259
TOTAL                               2062    2062  0.00%

Results for commit: 1f3932bc7a1f5536e47f28032a1b150214457777

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

@edelarua edelarua left a comment

Choose a reason for hiding this comment

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

Hi @BFalquet, please add nestcolor under Suggests in the DESCRIPTION file.

Copy link
Contributor

@edelarua edelarua left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@BFalquet BFalquet requested a review from barnett11 July 20, 2022 13:40
@pawelru
Copy link
Contributor

pawelru commented Jul 20, 2022

Addition of the nestcolor looks fine but I have two comments

  • You have added a lot of library(dplyr) (next to another already existing library(dm)) - is that needed? If examples run only functions from a package (and assuming that package prefixes / imports into the NAMESPACE everything it need) then that seems to be obsolete. It's needed only if you actually uses dplyr/dm functions in examples but that's not the case. For simplicity (as well as performance) I would suggest to remove it.
  • So the soft-dependency is correctly added but is it used? Do we have an example with plots (not tables)? If yes, then let's use it in there. Otherwise I would expect that some static code analysis should complain that dependent package is not being used at all. We probably need it in the long term but I am not sure if now but let's leave that.

@BFalquet
Copy link
Contributor Author

We will use it in mng01 which is (hopefully) about to be merged. For the library call, all the examples should be reformatted because they look terrible in the documentation. I ll open an issue for that.

@pawelru
Copy link
Contributor

pawelru commented Jul 20, 2022

I ll open an issue for that

Already there -> #223

So here I would suggest to revert library calls not to make it even more difficult.

@BFalquet BFalquet merged commit 3e9a019 into main Jul 20, 2022
@BFalquet BFalquet deleted the implement_nestcolor@main branch July 20, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants