search

eddelbuettel / rprotobuf

R Interface to Protocol Buffers
71 stars 25 forks source link

Use TextFormat in test_addressbook.R #99

Closed tanx16 closed 8 months ago

tanx16 commented 8 months ago

This resolves https://github.com/eddelbuettel/rprotobuf/issues/98.

eddelbuettel commented 8 months ago

Lovely, only two spots. Easy. To we have to version protect for older PB releases?

Also, would you mind adding your to file ChangeLog. Standard GNU / Emacs format of YYYY-MM-DD date, spaces, Name, spaces, email then entries after TAB * filename: description.

tanx16 commented 8 months ago

Done.

Do we have to version protect for older PB releases?

I'm not sure I understand. Are you concerned about using RProtoBuf with older versions of C++ Protobuf? toTextFormat() should be fully compatible with any C++ Protobuf version, as it's a stable API. C++ DebugString (and implicitly as.character and toString()) will eventually have a breaking change, but we don't have a set date for open source yet. When that happens, this change should allow RProtoBuf to update to the latest C++ version, but will likely break RProtoBuf users that happen to use as.character or toString() for serializing.

eddelbuettel commented 8 months ago

Yeah that was my question -- it was nice how we managed to move along from alonger pb2 to newer pb3. In practice, I suppose, most people will use current versions. And if toTextFormat() was always present it is less of an issue. Seems we were just sloppy when the test was written.

So big thanks for cleaning that up, and thanks also for obliging with the ChangeLog entry.

tanx16 commented 8 months ago

Happy to help. One alternative to breaking compatibility is to migrate the implementation of those APIs to depend on TextFormat, and instead create a new API that is explicitly a debug format.