search

pharmaR / riskassessment

Collaborative Deployment: https://app.pharmar.org/riskassessment/ Risk Assessment Demo App: https://rinpharma.shinyapps.io/riskassessment
https://pharmar.github.io/riskassessment/
Other
101 stars 28 forks source link

Add Package Dependencies to Reports, Part II #750

Closed Robert-Krajcik closed 4 months ago

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 80.00000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 81.60%. Comparing base (4fa6e61) to head (c84f59e).

:exclamation: Current head c84f59e differs from pull request most recent head 42a3255. Consider uploading reports for the commit 42a3255 to get more accurate results

Files Patch % Lines
R/utils_get_db.R 45.45% 6 Missing :warning:
R/mod_downloadHandler.R 94.44% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #750 +/- ## ========================================== - Coverage 81.61% 81.60% -0.02% ========================================== Files 34 34 Lines 5277 5301 +24 ========================================== + Hits 4307 4326 +19 - Misses 970 975 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

AARON-CLARK commented 7 months ago

@Robert-Krajcik, can you remind me what part II seeks to accomplish? Adding the suggests to the report downloader?

Robert-Krajcik commented 7 months ago

Hi @Aaron @.***> Yes, this adds sugggests

From: Aaron Clark @.> Sent: Tuesday, February 27, 2024 10:15 AM To: pharmaR/riskassessment @.> Cc: Robert Krajcik (EXTERNAL) @.>; Mention @.> Subject: [EXTERNAL] Re: [pharmaR/riskassessment] Add Package Dependencies to Reports, Part II (PR #750)

@Robert-Krajcik, can you remind me what part II seeks to accomplish? Adding the suggests to the report downloader? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned. Message ZjQcmQRYFpfptBannerStart This Message Is From an External Sender

EXTERNAL EMAIL: Use caution before replying, clicking links, and opening attachments. ZjQcmQRYFpfptBannerEnd

@Robert-Krajcikhttps://urldefense.com/v3/__https:/github.com/Robert-Krajcik__;!!LqrO4s96!npbUH2SIA0Xp889qiMbS2eTp6VTgoKWseVl6U4w7qAI17SYAg9OgS8Dc8bh0zVuBdi29A8RS60YH5CFNgoNIOkVZ84QF$, can you remind me what part II seeks to accomplish? Adding the suggests to the report downloader?

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/pharmaR/riskassessment/pull/750*issuecomment-1966919998__;Iw!!LqrO4s96!npbUH2SIA0Xp889qiMbS2eTp6VTgoKWseVl6U4w7qAI17SYAg9OgS8Dc8bh0zVuBdi29A8RS60YH5CFNgoNIOhiFg5Xp$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AR53IVWK7YINLTN537QKNIDYVYA67AVCNFSM6AAAAABCAZO6JCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWHEYTSOJZHA__;!!LqrO4s96!npbUH2SIA0Xp889qiMbS2eTp6VTgoKWseVl6U4w7qAI17SYAg9OgS8Dc8bh0zVuBdi29A8RS60YH5CFNgoNIOovxEB1U$. You are receiving this because you were mentioned.Message ID: @.**@.>>

Jeff-Thompson12 commented 6 months ago

@Robert-Krajcik was taking a quick look at this and noticed this on the reports. It's supposed to be "Suggests".

image
aclark02-arcus commented 6 months ago

@Robert-Krajcik, could you take a look at Jeff's finding? Thanks! Talk later today.

@Robert-Krajcik, I was taking a quick look at this and noticed this on the reports. It's supposed to be "Suggests".

Robert-Krajcik commented 6 months ago

Hi @AARON-CLARK Thanks for reviewing this. I am really swamped right now with a DMC project, but I will try to get to it by the end of the week.

aclark02-arcus commented 6 months ago

Hi @Robert-Krajcik, just checking in. Are you still swamped right now? Thanks!

Robert-Krajcik commented 6 months ago

Yes, still quite busy. I may have some time later today or tomorrow.

From: Aaron Clark @.> Sent: Monday, March 25, 2024 9:09 AM To: pharmaR/riskassessment @.> Cc: Robert Krajcik (EXTERNAL) @.>; Mention @.> Subject: [EXTERNAL] Re: [pharmaR/riskassessment] Add Package Dependencies to Reports, Part II (PR #750)

Hi @Robert-Krajcik, just checking in. Are you still swamped right now? Thanks! — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned. Message ID: pharmaR/riskassessment/pull/750/c2017973532@ github. com ZjQcmQRYFpfptBannerStart This Message Is From an External Sender

EXTERNAL EMAIL: Use caution before replying, clicking links, and opening attachments. ZjQcmQRYFpfptBannerEnd

Hi @Robert-Krajcikhttps://urldefense.com/v3/__https:/github.com/Robert-Krajcik__;!!LqrO4s96!hyWsqa7SF5tLRRy6EhJ0HzH6Zqz8u4Ggd2HmmR8Ks6CmPOi5Z995l7dZDZ-Fn4VB1ar9qMFdgvWIwUa_AkUho-edckfX$, just checking in. Are you still swamped right now? Thanks!

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/pharmaR/riskassessment/pull/750*issuecomment-2017973532__;Iw!!LqrO4s96!hyWsqa7SF5tLRRy6EhJ0HzH6Zqz8u4Ggd2HmmR8Ks6CmPOi5Z995l7dZDZ-Fn4VB1ar9qMFdgvWIwUa_AkUho3z8hZ8H$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AR53IVULR674VEZOU4N3TA3Y2AOVZAVCNFSM6AAAAABCAZO6JCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJXHE3TGNJTGI__;!!LqrO4s96!hyWsqa7SF5tLRRy6EhJ0HzH6Zqz8u4Ggd2HmmR8Ks6CmPOi5Z995l7dZDZ-Fn4VB1ar9qMFdgvWIwUa_AkUho_bi1_U2$. You are receiving this because you were mentioned.Message ID: @.**@.>>

jthompson-arcus commented 5 months ago

HI @Robert-Krajcik, this is great so far. I second @Jeff-Thompson12's discovery regarding the report's mis-labeled card. I also noticed that the report doesn't have enough space for the Package Dependencies table sometimes. Here's an example using tidyCDISC. Notice how the table is printed / rendered over top of the comments section:

image

@aclark02-arcus I'm not seeing this behavior. Can you check to see if this is still happening on your end?

aclark02-arcus commented 5 months ago

Hi, looks good on my end too!

jthompson-arcus commented 5 months ago

@aclark02-arcus I forget. Didn't we also want to go ahead and add introJS in this PR or another?

jthompson-arcus commented 5 months ago

@aclark02-arcus I looked at adding the cross functionality between the download reports and the dependency tab. It's definitely doable but I don't really want to add it into this PR. I also would rather add the introJS in another PR. This one is ready for review.

jthompson-arcus commented 5 months ago

First, I feel like calling the top two (Dependencies Uploaded and Type Summary) a "Metric" may be inappropriate. That is, the number of pkgs uploaded & the spread of dependencies is useful info, but they don't help me make an dependency-based decision for any package.

I don't really agree with this regarding the type summary. Even just regarding the dependencies, R deals with those packages in different ways. An imported package dependency may only need to be validated against what it is specifically importing whereas a depends dependency has a different ramification to your R environment. I agree with you regarding the number of dependencies uploaded. It makes more sense in the application itself than in the reports.

"% Base R" does seem like a metric.

I agree with this. Maybe what would be more helpful is if we had categories of automatically validated packages (e.g. base R and maybe tidyverse). I don't feel like these line items have to be metrics per se, but they should carry information useful in decision making around validation.

Thoughts?

My thoughts are that we've been so focused on the app, but it's about time we reassessed the reports. Pretty much only the HTML report looks nice too which is a shame.