ensure inheritanceDT is nonempty before doing comparison. Fixes #256

[TylerGrantSmith]

Just added a simple row count check before comparing data table values. Otherwise the result of the data.table command is NA

[codecov-io]

Codecov Report

Merging #262 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #262   +/-   ##
=======================================
  Coverage   92.44%   92.44%           
=======================================
  Files          12       12           
  Lines         927      927           
=======================================
  Hits          857      857           
  Misses         70       70

Impacted Files	Coverage Δ
R/FunctionReporter.R	93.23% <100%> (ø)	⬆️

---

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 2f43f73...fcee412. Read the comment docs.

[jameslamb]

@TylerGrantSmith thanks for the fix, looks good to me! I'm guessing this showed up for you as an error like

Error in if (NA) { : missing value where TRUE/FALSE needed

Would you be willing to add a unit test that reproduces this situation, so that we can be sure we don't reintroduce the bug?

[TylerGrantSmith]

@jameslamb I got the same error that was originally listed in #256
## Error in if (is.na(out) && inheritanceDT[CLASS_NAME == parent_name, !is.na(PARENT_NAME) && : missing value where TRUE/FALSE needed

I will see what I can do about the unit test. I have never made a test for an R6 method, so I will try to base it off what you all have done in the other tests.

[jayqi]

I think the way to test this is to have some class A's method depend on an inherited class B's method, where class B is in a different package.

I feel like this may end up being a somewhat involved test to write (we'd need two test packages? or make a new test package depend on an existing test package?), so feel free to open an issue about it to capture the need, and we can just merge this PR as is so it's not blocked.

[TylerGrantSmith]

I feel like this may end up being a somewhat involved test to write (we'd need two test packages? or make a new test package depend on an existing test package?)

I agree, and that is why I haven't worked on the test yet. It does raise a methodological question for me though. If you were to take the latter path and modify an existing test package, what checks are there to guarantee it doesn't "break" the tests it was originally designed for.

For example, with with the silverstein test package in #263, the structure of the classes is fairly specific and it would be easy to modify the package in a way that would not cause the associated test to fail, but the test would not be working as intended.

[jayqi]

@TylerGrantSmith I've opened #265 to document this missing test. Let's move the discussion there. Thanks for your PR!