added node coloring in DependencyReporter (fixes #238, #239)

[jameslamb]

In this PR, I propose one way we could accomplish #238 and #239

[codecov-io]

Codecov Report

Merging #243 into master will decrease coverage by 1.45%. The diff coverage is 83.01%.

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
- Coverage   92.46%   91.01%   -1.46%     
==========================================
  Files          12       12              
  Lines         929      957      +28     
==========================================
+ Hits          859      871      +12     
- Misses         70       86      +16

Impacted Files	Coverage Δ
R/DependencyReporter.R	82.88% <100%> (+3.09%)	:arrow_up:
R/AbstractGraphReporter.R	81.28% <73.52%> (-8.72%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9fe580d...ead2b68. Read the comment docs.

[jameslamb]

Alright @jayqi @bburns632 I think this is ready for review!

To test, you can run this code

WHERE_YOUR_REPO_IS <- "~/repos/pkgnet/"
devtools::load_all(WHERE_YOUR_REPO_IS)

pkg_name <- 'ggplot2'
reporter <- DependencyReporter$new()
reporter$set_package(
    pkg_name = pkg_name
)
reporter$graph_viz

CreatePackageReport(
    pkg_name = pkg_name
)

I could not figure out how to generate a legend. Simply adding visNetwork::visLegend() wasn't working for me. So for now I propose settling for just describing the colors in text on the Dependency Report page and then opening an issue to add the legend later. But @bburns632 @jayqi you're both better at visNetwork than me so lmk if you see an easy way to add one in this PR!

[jameslamb]

Hey friends! Could I get a review on this, at your earliest convenience?

[jameslamb]

Hey friends! Could I get a review on this, at your earliest convenience?

👀

[jameslamb]

@bburns632 @jayqi could I get a review on this?

[jameslamb]

@bburns632 @jayqi could I get a review on this?

@bburns632 @jayqi

[bburns632]

James,

I am so sorry for being so terrible with this review. Unfortunately, I got a new phone, forgot to transfer the 2FA, and am now in the process of getting new ones for many sites including github. As soon as I get re-set up, I'll mash that approval button hard.

SORRY!

On Mon, Jan 6, 2020 at 10:13 PM James Lamb notifications@github.com wrote:

@bburns632 https://github.com/bburns632 @jayqi https://github.com/jayqi could I get a review on this?

https://camo.githubusercontent.com/5dd086b507172b21abc15cc98bde24d32d979d10/68747470733a2f2f6d656469612e67697068792e636f6d2f6d656469612f636d725763774333413472794834333752752f67697068792e676966

@bburns632 https://github.com/bburns632 @jayqi https://github.com/jayqi

https://camo.githubusercontent.com/7908da38ab8fb5a26e522b4e041aae5325c716ef/68747470733a2f2f6d656469612e67697068792e636f6d2f6d656469612f615a34735155707962616935792f67697068792e676966

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uptake/pkgnet/pull/243?email_source=notifications&email_token=AF3FDS4PBSS3WECCL4DXNVDQ4P6O7A5CNFSM4IZZM7TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIHUD5I#issuecomment-571425269, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF3FDS6GZMCWAHQYC3QKS6DQ4P6O7ANCNFSM4IZZM7TA .

[jameslamb]

Ha no problem! I'm having fun finding Agnes GIFs to put on here

[bburns632]

@jameslamb Two things:

1. I get this error "## Error in setkeyv(resultDT, "node_name"): some columns are not in the data.table: node_name" printed directly in the report. That needs to be fixed.
2. I second @jayqi 's comment on color choice. As it stands, the mundane "in every R installation" is colored the most vibrantly. IMO that should be in the bleh gray or tope or something. What about something along the lines of target repo = black, regular R packages = light gray, and other packages the light green or something else catchy?

[bburns632]

I auto-approved then looked at it since I drug this review out so long... 😬

[jameslamb]

@jameslamb Two things:

1. I get this error "## Error in setkeyv(resultDT, "node_name"): some columns are not in the data.table: node_name" printed directly in the report. That needs to be fixed.
2. I second @jayqi 's comment on color choice. As it stands, the mundane "in every R installation" is colored the most vibrantly. IMO that should be in the bleh gray or tope or something. What about something along the lines of target repo = black, regular R packages = light gray, and other packages the light green or something else catchy?

sure I can do number 2. What code did you run that produced the error you see in 1?

[jameslamb]

Just updated! Here's what it's looking like for ggplot2

@bburns632 I generated this by running the code from https://github.com/uptake/pkgnet/pull/243#issuecomment-544779752, except that I installed the package and library()'d it instead of devtools::load_all(), and I didn't get the error you reported. So maybe just installing the package fixes that error?

[jayqi]

Looks great! Wondering if we should add a legend. visNetwork's legends look terrible but maybe still better to have one.

[jameslamb]

Looks great! Wondering if we should add a legend. visNetwork's legends look terrible but maybe still better to have one.

I totally agree with you, but like I said in https://github.com/uptake/pkgnet/pull/243#issuecomment-544779752 I personally don't know how to add legend :/

[bburns632]

Is there a vizLegend command?

https://datastorm-open.github.io/visNetwork/legend.html

On Fri, Jan 31, 2020, 1:07 AM Jay Qi notifications@github.com wrote:

@jayqi commented on this pull request.

In R/AbstractGraphReporter.R https://github.com/uptake/pkgnet/pull/243#discussion_r373340927:

@@ -291,80 +291,86 @@ AbstractGraphReporter <- R6::R6Class(

         ## Color Nodes

• Flag for us to do stuff later

• This flag controls whether nodes are colored by categorical

• groups or some continuous attribute

       colorByGroup <- FALSE

(Also sorry about missing your comment regarding the legend before.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uptake/pkgnet/pull/243?email_source=notifications&email_token=AF3FDS2JQPAR5MEUXHGTM5TRAPE2NA5CNFSM4IZZM7TKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTYMJMQ#discussion_r373340927, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF3FDSY4KFB3K4LQUYZ7MW3RAPE2NANCNFSM4IZZM7TA .

[jameslamb]

Is there a vizLegend command? https://datastorm-open.github.io/visNetwork/legend.html … On Fri, Jan 31, 2020, 1:07 AM Jay Qi @.> wrote: @*.** commented on this pull request. ------------------------------ In R/AbstractGraphReporter.R <#243 (comment)>: > @@ -291,80 +291,86 @@ AbstractGraphReporter <- R6::R6Class( ## Color Nodes - # Flag for us to do stuff later + # This flag controls whether nodes are colored by categorical + # groups or some continuous attribute colorByGroup <- FALSE (Also sorry about missing your comment regarding the legend before.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#243?email_source=notifications&email_token=AF3FDS2JQPAR5MEUXHGTM5TRAPE2NA5CNFSM4IZZM7TKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTYMJMQ#discussion_r373340927>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF3FDSY4KFB3K4LQUYZ7MW3RAPE2NANCNFSM4IZZM7TA .

yes there is. As I mentioned in https://github.com/uptake/pkgnet/pull/243#issuecomment-544779752, I wasn't able to get it to work. I should have been more specific than just "couldn't get it working" because now I don't remember.

It sounds like you and @jayqi really really want a legend to be part of this PR...totally fair. Let me check it out this weekend.

[jayqi]

Regardless of if we want a legend or not, there is some leftover old code in the plot_network function that you should clean up, because it may lead to (or already is leading to) bugs.

[jameslamb]

Ok think I've addressed the comments in https://github.com/uptake/pkgnet/pull/243/commits/84753ef7c3b740910184144f1da3d416ec6a4ed1. Take a look!

The legend now looks like this:

The addNodes part of visLegend is mostly undocumented, so I couldn't figure out how to change the font color on individual things in the legend. I changed the package in focus to orange (from black) because of that.

I chose to stick with ellipses after struggling for a while with circles. It was hard to get the circle text to resize, so I was ending up with circles that were overlapping or different sizes.

[jameslamb]

blegh yeah I saw that on Travis. Looking!

[jameslamb]

Sorry for the long delay! I think this is working as expected!

Try installing from this branch and running CreatePackageReport, such as:

CreatePackageReport(
    pkg_name = "pkgnet"
    , pkg_reporters = list(
        pkgnet::FunctionReporter$new()
        , pkgnet::DependencyReporter$new()
        , pkgnet::InheritanceReporter$new()
    )
)

https://github.com/uptake/pkgnet/pull/243#pullrequestreview-351901491

I also just rebased to master to get the latest changes.

[jameslamb]

I think this is now working as expected. Could I get another review?

[jameslamb]

I just rebased to master to get the latest changes.

[bburns632]

LGTM.