Proposal: Prevent set_package after a package has already been set

[jayqi]

Right now, we have a lot of calculated stuff cached in the Reporters, e.g., nodes, edges, metrics. If a user overwrites the package by calling set_package again, then it invalidates those cached objects and so we have functionality to reset_cache and wipe them out.

This has a downside in that having to worry about cache resetting adds complexity. As an example, it has led to bug #70.

I propose that instead, we prevent users from overwriting the package set for a Reporter (i.e., if private$private_pkg_name exists, throw a fatal error if they try to use set_package).

[jayqi]

@jameslamb @bburns632 especially would like your input here.

To briefly summarize some discussion I had with @jameslamb last night: he asked why not just put the package in the constructor. We didn't want to do that because we want to instantiate DefaultReporters without specifying a package. @ngparas suggested we consider a builder pattern

[jayqi]

I think probably a simpler solution is to just make the package setting part of the the constructor.

We can easily define DefaultReporters like:

DefaultReporters <- function() {
    return(list(
          SummaryReporter
        , DependencyReporter
        , FunctionReporter
    ))
}

and then this part of .BuildPackageReporters we instead have

pkg_reporters <- sapply(
          X = pkg_reporters
          , FUN = function(reporter){
              reporter$new(pkg_name, pkg_path)
          }
      )

Of course, this is a breaking change. If we decide to do this, we should do it sooner rather than later.

[bburns632]

@jayqi with the significant changes you made this past month or two, do you see this change as more, less, or equally valuable as when it was proposed?

[jameslamb]

@bburns632 I just talked through this with @jayqi .

I argued that we should NOT change DefaultReporters to a list of generators. The reason we used this thing$set_package() pattern is that it allows you to write reporters that have different constructors with arguments specific to what they do and then have the orchestrating thing (CreatePackageReport()) re-use them. So, for example, PackageDependencyReporter exposes an argument dep_types in its constructor.

If we were to do the "pass in class generators" thing, you would have to expose dep_types in the signature of CreatePackageReport or change the list input to have some combination of class generators and named parameters.

I propose that we keep the pattern we have (this is a well-known design pattern called dependency injection) but that we reduce the complexity of the code by revoking the ability to reset the package on a Reporter once it's been set.