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

Class Inheritance Reporter #129

Merged
merged 22 commits into from
Dec 3, 2018

Conversation

jayqi
Copy link
Collaborator

@jayqi jayqi commented Nov 17, 2018

Closes #14

Summary:

  • New InheritanceReporter that extracts the inheritance network for S4, Reference, and R6 classes within a given package.
  • Changed FunctionReporter to explicitly exclude generator functions for Reference classes for now.
    I'm not sure the best way to represent object construction in FunctionReporter yet.
  • Added some S4 and Reference classes to milne for testing.
  • Changed all pallete to correct spelling palette.

Viz for pkgnet:
pkgnet_inheritance

Viz for milne
milne_inheritance

@jayqi jayqi force-pushed the feature/inheritance-reporter branch from 3286e59 to 3842a55 Compare November 17, 2018 23:18
@jayqi jayqi changed the title (WIP) Class Inheritance Reporter Class Inheritance Reporter Nov 30, 2018
Copy link
Collaborator

@bburns632 bburns632 left a comment

Choose a reason for hiding this comment

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

@jayqi , I am excited to see results from these two tests:

devtools::install('.')

devtools::install("./inst/milne")
t3 <- InheritanceReporter$new()
t3$set_package(pkg_name = 'milne')
t3$edges
t3$graph_viz 

t4 <- InheritanceReporter$new()
t4$set_package(pkg_name = 'lubridate')
t4$edges
t4$graph_viz 

Here's the lubridate graph.
image

However, I cannot reference this as a reporter through our high level function CreatePackageReport().

pkgnet::CreatePackageReport('lubridate', pkg_reporters = c(DefaultReporters, InheritanceReporter)) # error
pkgnet::CreatePackageReport('milne', pkg_reporters = c(DefaultReporters, InheritanceReporter)) # error

In both cases, I get the error "At least one of the reporters in the pkg_reporters parameter is not a PackageReporter. See ?AbstractPackageReporter for details." stemming from this test: https://github.com/UptakeOpenSource/pkgnet/blob/master/R/CreatePackageReport.R#L37

Can we enable this reporter to be used in this fashion (a specific call out in CreatePackageReport) before we merge this PR? Then, in a later R, we could automate this with an R6::is.R6() check.

@jayqi
Copy link
Collaborator Author

jayqi commented Nov 30, 2018

@bburns632 Ironically, you are getting the errors because you are using both CreatePackageReport and DefaultReporters incorrectly. (Relevant to the discussion of getting rid of set_package in #106.)

The following works for me:

pkgnet::CreatePackageReport('lubridate'
                            , pkg_reporters = c(DefaultReporters()
                                                , InheritanceReporter$new()
                                                ))
pkgnet::CreatePackageReport('milne'
                            , pkg_reporters = c(DefaultReporters()
                                                , InheritanceReporter$new()
                                                ))

The key two things are:

  • DefaultReporters is a function and not directly a list of reporters
  • Inputs to CreatePackageReport must be initialized reporter objects, not the generators

cc: @jameslamb

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Looks great, Jay! I added some suggestions for changed. @ me when you want me to review again.

Overall I agreed with the approach and the core of the implementation. My recommendations are mostly about hardening it a bit to avoid subtle errors or pedantic stuff like whitespace.

.gitignore Outdated Show resolved Hide resolved
R/AbstractGraphReporter.R Show resolved Hide resolved
R/PackageFunctionReporter.R Show resolved Hide resolved
R/PackageFunctionReporter.R Show resolved Hide resolved
R/PackageInheritanceReporter.R Show resolved Hide resolved
R/PackageInheritanceReporter.R Show resolved Hide resolved
inst/milne/R/Swing_Song.R Outdated Show resolved Hide resolved
inst/milne/R/The_Friend.R Show resolved Hide resolved
tests/testthat/test-plotting.R Show resolved Hide resolved
inst/package_report/package_inheritance_reporter.Rmd Outdated Show resolved Hide resolved
@jayqi jayqi force-pushed the feature/inheritance-reporter branch from ef234a1 to 4b43991 Compare November 30, 2018 21:49
@jayqi jayqi force-pushed the feature/inheritance-reporter branch from 4b43991 to 7cad3bf Compare November 30, 2018 21:49
@jayqi
Copy link
Collaborator Author

jayqi commented Nov 30, 2018

@jameslamb I think I have resolved all of your comments.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

LGTM

@bburns632
Copy link
Collaborator

Yes, the irony is delicious here. Clearly, this is not an issue with this reporter, but a UX issue to bring up later regarding using speciality or custom reporters (now that we have a new one!).

All changes look good to me.

@bburns632 bburns632 merged commit 87a924e into uptake:master Dec 3, 2018
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.

None yet

3 participants