search

r-lib / cpp11

cpp11 helps you to interact with R objects using C++ code.
https://cpp11.r-lib.org/
Other
199 stars 46 forks source link

interface around {fmt} for `cpp11::stop()` and cpp11::warning()` message formatting #209

Closed sbearrows closed 3 years ago

sbearrows commented 3 years ago

@jimhester Let me know what you think! Tests are pretty simple but we can add more if issues arise.

Fixes #169

jimhester commented 3 years ago

So I was thinking about this some this morning, and I think we will either have to use new function names (something like stop_fmt() for the fmt powered functions, or put them behind a #ifdef CPP11_USE_FMT flag that you have to opt into to use them.

This is because existing packages that are using cpp11::stop() are going to be using the sprintf style %s etc. message strings, and these won't work with fmt.

Another argument in favor of the #ifdef approach is that compiling the fmt code will incur some overhead, which some people may want to avoid.

Which way do you think would be best?

sbearrows commented 3 years ago

I'd be in favor of the later! I think decreasing overhead is probably important to a lot of people using cpp11. What about you?

jimhester commented 3 years ago

yeah that is the way I was leaning, though I worry if it is only opt-in then people may not realize it is an option / never use it.

sbearrows commented 3 years ago

We could try to make a ruckus about? Adding examples in documentation?

Also, could we make fmt for cpp11::stop() and cpp11::warning() opt-in but make fmt for the new cpp11::messages() opt-out since it isn't present in previous versions? This might give people some breadcrumbs for the others? Not sure if that would be too confusing though.

jimhester commented 3 years ago

LGTM, I think this can be merged once the tests are green.