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

R6 Methods in FunctionReporter #128

Merged
merged 11 commits into from
Nov 30, 2018
Merged

Conversation

jayqi
Copy link
Collaborator

@jayqi jayqi commented Nov 14, 2018

Resolves #123

Summary of changes:

  • Added support for R6 class methods in FunctionReporter. Some design choices/constraints:
    • Class names are the name of the generator object in the package namespace, and not the classname attribute. This is because the classname attribute is not required to be defined and also not required to match the generator object name, though people conventionally do assign one with the same name.
    • Nodes have names of the form <classname>$<methodtype>$<methodname>, e.g., FunctionReporter$private_methods$extract_nodes .
    • Methods only ever show up as one node even with inheritance, unless the child class overrides with a new definition. Inherited methods are assigned to the class in which they were defined, and not for any child classes
    • Dependency on calls to instantiated objects' methods are not supported. Instantiated objects can have arbitrary names and I don't want to deal with it right now.
  • Added test package milne for testing R6 classes
  • Changed the .gitignore entries for data files to only exclude data files in package root. Want to be able to commit test data in tests. Added .gitignore exclusion for csv files in tests/testthat/testdata directory.

Not addressed in this PR. Maybe for discussion in issues after merging.

  • Calls to object constructors by functions are ignored right now, like FunctionReporter$new() in DefaultReporters? Not clear the right way to represent these. Does that edge link to the initialize method for that class? FunctionReporter has no initialize method, so would we link the edge to the initialize method inherited up the ancestor sequence? But actually none of its ancestors have an initialize method defined -- they're all using the vanilla one built-in to R6. So there isn't even a node right now for it.

@jayqi jayqi changed the title WIP: R6 Methods in FunctionReporter R6 Methods in FunctionReporter Nov 17, 2018
@jayqi jayqi mentioned this pull request Nov 17, 2018
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.

This is awesome! I added a few comments

.gitignore Outdated Show resolved Hide resolved
R/PackageFunctionReporter.R Show resolved Hide resolved
R/PackageFunctionReporter.R Outdated Show resolved Hide resolved
R/PackageFunctionReporter.R Show resolved Hide resolved
R/PackageFunctionReporter.R Show resolved Hide resolved
R/PackageFunctionReporter.R Show resolved Hide resolved
tests/testthat/test-functional_structure.R Outdated Show resolved Hide resolved
R/PackageFunctionReporter.R Outdated Show resolved Hide resolved
R/PackageFunctionReporter.R Show resolved Hide resolved
@jayqi
Copy link
Collaborator Author

jayqi commented Nov 24, 2018

@jameslamb Addressed your comments! Thanks for reviewing.

@jameslamb
Copy link
Collaborator

This is good with me! Will leave it to @bburns632 to do one more pass and and smash merge, since this is a decent-sized PR

@bburns632
Copy link
Collaborator

@jayqi this PR brings to light the bottom depths of an R package. It is the next level of insight into a package. Excellent work.

AND I look forward to PR #129 to add some structure to these newly surfaced elements.

Here's master currently for a pkgnet report on pkgnet:

image

Here's the same report with changes from this PR:
image

@bburns632 bburns632 merged commit a329f56 into uptake:master Nov 30, 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