-
Notifications
You must be signed in to change notification settings - Fork 31
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
Incorporate Reverse Dependency Improvements #753
Conversation
…nto jt-746-impr_rev_dep
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #753 +/- ##
==========================================
+ Coverage 79.23% 81.27% +2.03%
==========================================
Files 33 33
Lines 5114 5169 +55
==========================================
+ Hits 4052 4201 +149
+ Misses 1062 968 -94 ☔ View full report in Codecov by Sentry. |
@Jeff-Thompson12 I implemented suggested changes on my own fork since that's where I have write access; see #751. |
Add feature to show reverse dependencies of selected package that are currently in database
2cf171f
to
813071c
Compare
@AARON-CLARK & @Robert-Krajcik I still haven't done a deep dive into the code add by this PR. I would like your input on how the output is being produced. For example, consider the two tidyverse packages of {dplyr} and {tibble}. {tibble} is in the imports of {dplyr}, and {dplyr} is in the suggests of {tibble}. Currently the application outputs both as reverse dependencies of each other. The table currently does not include a type for reverse dependency and we could include it but I it might be confusing. Then we would have a table where package: tibble and type: imports would tibble is in the imports of {package} but the second table it would mean {package} is in the imports of tibble. Thoughts? |
@LDSamson I granted you write permissions! Done ✅ |
@Jeff-Thompson12, I would think adding a "type" column could make things slightly confusing, but maybe not. What if we moved the new table below the text output of "All reverse deps"? And we could make the "Reverse Deps" header larger. That may help folks separate the two tables in their minds. Regardless if it's added or not, I did notice that |
@aclark02-arcus It's ready for you to take another look. @LDSamson Sorry it's taken so long to get this PR ready to review. If some desired functionality was lost, feel free to request changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jthompson-arcus, I'm not sure if this is happening for you, but I'm getting a blank Package Dependency tab when tidyCDISC
is selected on the control panel. Can you please verify?
Thanks for picking up this PR and improving it, I am happy to see that you find it useful. I will have a look this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing after recent commits, everything looks good! Thanks @LDSamson & @jthompson-arcus! Merging this one. If there is anything else missing, it could be incorporated as a new PR.
Addresses #746 and incorporates #751