eddelbuettel / rprotobuf

R Interface to Protocol Buffers
71 stars 25 forks source link

Support waldo comparisons of RProtoBuf objects #78

Closed michaelquinn32 closed 3 years ago

michaelquinn32 commented 3 years ago

Using the third edition of testthat means also using the waldo package. It has stricter rules about comparing external pointers. Objects that refer to different locations in memory can't be considered equal. To work around this, waldo supports comparison proxies: https://github.com/r-lib/waldo/blob/master/R/proxy.R

They currently export methods for data tables and XML objects, both of which contain external pointers. Since all protobuf objects have a well-defined string representation, I believe that using obj$toString() would be a suitable proxy.

I can submit a PR if this works for you.

eddelbuettel commented 3 years ago

Hm, but we already have a toString exporter. Would the task then not be in waldo or testthat, or the user of tests, by creating string objects to be compared?

michaelquinn32 commented 3 years ago

Thanks for the quick reply!

The fix involves adding several methods for compare_proxy, e.g.

compare_proxy.Message <- function(x) {
  return(x$toString())
}

And so on for all of the classes that support $toString().

I'm not sure what the better place would be for that. We could either have these here or I could move the PR over to the waldo package. Please let me know.

eddelbuettel commented 3 years ago

Thanks for the clarification. "Rounding out" the API is definitely something we can do, I was mostly a little defensive as your first post could also be read as implying more dependencies would be needed to satisfy a corner case.

But yes, the amazing thing with S4 classes is that ... you always find a few more you should have added last time. And as you probably have a test case we can surely look into that.

michaelquinn32 commented 3 years ago

I can definitely do that.

The only open question is whether or not you want the waldo Import here, for the generic. If not, I can always ask the waldo maintainers if they want this change there.

eddelbuettel commented 3 years ago

I am a somewhat strong believer in being economical with dependencies so I would have reservations about pulling waldo in. (Never mind that I wrote a similar package wrapping diffobj for use from test runners which we did for both tests, and their use in a grading framework we use for a class I teach.)

eddelbuettel commented 3 years ago

That appears to have been taken care of over there.

michaelquinn32 commented 3 years ago

Thanks for the help! And thanks for wrapping this up!