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

various enhancements #43

Merged
merged 20 commits into from
May 20, 2024
Merged

various enhancements #43

merged 20 commits into from
May 20, 2024

Conversation

pawelru
Copy link
Contributor

@pawelru pawelru commented Apr 19, 2024

these are various enhancements that I have collected over a time of running verdepcheck on our codebase. That includes:

  • docs
  • edge-cases fixes
  • get_ref_release will not return early but rather collects candidates and return ref for which ver is the maximum ver over candidates
  • get_release_date.remote_ref_github will use pkgdepends to resolve ref and get commit sha and reach GH with that sha
  • use binaries. This hardcodes the repos but it's fine for the time being. We are anyway relaying on PPM when using snapshots.

Copy link
Contributor

github-actions bot commented Apr 19, 2024

badge

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ------------------------
R/check.R                            37      37  0.00%    26-170
R/deps_installation_proposal.R      139     139  0.00%    58-269
R/desc_utils.R                       71      43  39.44%   15-32, 108, 117-168, 186
R/get_ref.R                         233     232  0.43%    13-455, 473-516
R/solve.R                            84      84  0.00%    9-170
R/utils.R                            66      42  36.36%   3-23, 41, 47, 57-104
TOTAL                               630     577  8.41%

Diff against main

Filename          Stmts    Miss  Cover
--------------  -------  ------  --------
R/desc_utils.R       +1      +1  -0.56%
R/get_ref.R         +15     +15  -0.03%
R/solve.R            +3      +3  +100.00%
R/utils.R            -4     -13  +14.94%
TOTAL               +15      +6  +1.26%

Results for commit: 8a41033

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Apr 19, 2024

Unit Tests Summary

 1 files   4 suites   2s ⏱️
41 tests 13 ✅ 28 💤 0 ❌
94 runs  65 ✅ 29 💤 0 ❌

Results for commit 8a41033.

♻️ This comment has been updated with latest results.

@pawelru
Copy link
Contributor Author

pawelru commented Apr 22, 2024

@averissimo please have a look if you can but this is not urgent

Copy link
Contributor

github-actions bot commented May 13, 2024

CLA Assistant Lite bot ✅ All contributors have signed the CLA

@pawelru
Copy link
Contributor Author

pawelru commented May 13, 2024

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

@averissimo averissimo self-assigned this May 14, 2024
@averissimo
Copy link
Contributor

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

@pawelru
Copy link
Contributor Author

pawelru commented May 14, 2024

@averissimo
Copy link
Contributor

This works for most cases, jsonlite on {teal.data} (and I suppose in others) is still trying to install the source version as the binary is not available in snapshots for versions prior to 2.8.7 (in snapshots from 2023-06-30).

We could bump rmarkdown to 2.23 (from 2023-07-01) which will solve this particular problem.

@pawelru
Copy link
Contributor Author

pawelru commented May 15, 2024

I just did that (insightsengineering/teal.data@63e5b4b) and it all passed -> https://github.com/insightsengineering/teal.data/actions/runs/9093304868

One great outcome of this PR is that it's a little bit faster when binaries were used. The aforementioned successful run took 6m 24s whereas the latest finished one - 9m 16s.

This is ready for review.

Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

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

The enhancements are very nice!

I suggested some extra cleanup. I can take care of it

The binary preference on cohort and isolate is quite interesting and will speed up the actions as well as avoid weird problems 👍

R/get_ref.R Show resolved Hide resolved
R/get_ref.R Outdated Show resolved Hide resolved
R/get_ref.R Outdated Show resolved Hide resolved
R/get_ref.R Outdated Show resolved Hide resolved
R/get_ref.R Show resolved Hide resolved
R/get_ref.R Show resolved Hide resolved
@averissimo
Copy link
Contributor

averissimo commented May 15, 2024

I have all 19 local branches that have rmarkdown (>=2.19) updated to 2.23 and ready to push to create PRs. (bash was a timesaver here)

Also detected that rmarkdown is missing from:

teal.widgets, teal.modules.general, citril.metadata, osprey and teal.goshawk.

These packages have knitr, but not rmarkdown

pawelru and others added 3 commits May 16, 2024 11:36
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Signed-off-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Signed-off-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com>
@pawelru
Copy link
Contributor Author

pawelru commented May 16, 2024

I have all 19 local branches that have rmarkdown (>=2.19) updated to 2.23 and ready to push to create PRs. (bash was a timesaver here)

Also detected that rmarkdown is missing from:

teal.widgets, teal.modules.general, citril.metadata, osprey and teal.goshawk.

These packages have knitr, but not rmarkdown

Thanks. Would you able to open PRs for these?

I just pushed changes with regards to the review comments. Unfortunately this is still WIP as the tests are failing. I will continue on this tomorrow

@pawelru pawelru requested a review from averissimo May 17, 2024 09:29
Copy link
Contributor

github-actions bot commented May 17, 2024

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
get_ref 💀 $0.01$ $-0.01$ get_cran_data_returns_date_for_Bioconductor
get_ref 👶 $+0.02$ get_release_data_returns_date_for_Bioconductor
get_ref 👶 $+0.03$ get_release_date.remote_ref_cran_will_retrieve_missing_date_NA_for_package.does.not.exist_1.1.0
get_ref 💀 $0.03$ $-0.03$ get_release_date.remote_ref_cran_will_retrieve_missing_date_NA_for_rlang_0.0.0
get_ref 👶 $+0.05$ get_release_date.remote_ref_github_will_retrieve_missing_date_NA_for_r_lib_rlang_v0.0.0
get_ref 💀 $0.03$ $-0.03$ get_release_date.remote_ref_github_will_retrieve_missing_date_NA_for_rlang_0.0.0

Results for commit 62e964c

♻️ This comment has been updated with latest results.

Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

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

This is a step ahead. Great work!

@averissimo averissimo merged commit 5ee21e1 into main May 20, 2024
29 checks passed
@averissimo averissimo deleted the enhancements branch May 20, 2024 23:55
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants