search

Rblp / Rblpapi

R package interfacing the Bloomberg API from https://www.bloomberglabs.com/api/
Other
167 stars 75 forks source link

Switch from knitr+rmarkdown+minidown to simplermarkdown #350

Closed eddelbuettel closed 2 years ago

eddelbuettel commented 2 years ago

This PR suggests to follow a practice I just introduced in six of my own packages to replace use of packages knitr, rmarkdown and (for the CSS formatting) minidown (which taken together have a bit of footprint) with the much simpler new package simplermarkdown which only needs one other package (for JSON) (plus a tiny CSS file) and wraps pandoc directly. See the reverse suggests of simplermarkdown for examples.

Our vignette only used pandoc for code formatting and actually ran no R code (which is not uncommon) so the switch was trivial. I put a locally rendered html version of the vignette here if you want to peruse it.

As for motivation, I find this (done via package deepdep) quite illustrative:

image

All that said, this is of course not required as a change but merely reflects my usage preference and experience. We can easily leave things as they are for no harm. So if the majority vote is "no change" we can close this PR.

johnlaing commented 2 years ago

I'm strongly in favor of this. Over the weekend I got the package up and running on cygwin at home and have some fresh wounds from knitr-related dependency hell.

eddelbuettel commented 2 years ago

Yep. As they "when you know, you know".

johnlaing commented 2 years ago

If you don't mind, let me try running it (later this evening) before approving.

eddelbuettel commented 2 years ago

By all means! I have done the same (doing the conversion here in code first, and then one package (to CRAN) at a time).

I also noticed our Travis setup is dead (as it is everywhere else). I have a working (and again, simpler / smaller / more minimal than other folks) setup in r-ci that I can bring easily to GitHub Actions. I'll file a separate ticket for that.

eddelbuettel commented 2 years ago

And if and when you approve (not jumping the gun, just thinking out loud) it may make sense to roll this to CRAN too as we did in fact get real bug fixes in in this last cycle.

johnlaing commented 2 years ago

Approved/merged. And yes to deploying a new version to CRAN.