Bioconductor / BiocStyle

Issues and pull requests for BiocStyle should go here.
12 stars 19 forks source link

Triple colons #18

Open yihui opened 7 years ago

yihui commented 7 years ago

It seems Bioconductor, unlike CRAN, is more friendly to :::. In general, it is a bad idea, but I confess I use it, too. However, I only use it when I feel I can grab my phone and bother the package maintainer immediately whenever there is a problem, e.g. the maintainer is myself, my colleague, or a close friend.

I found by chance that this package made heavy use of :::. Unfortunately, these internal functions in other people's package are not guaranteed to be stable. They are not exported for reasons. Basically no public API means no responsibility. For example, you will find BiocStyle::html_document will be broken with the next version of rmarkdown due to changes in rmarkdown:::pandoc_html_highlight_args (you can test the current dev version on Github).

If the only things you want to tweak are the CSS and the template, you may well use the css and template arguments, e.g.

output:
  html_document:
    css: !expr BiocStyle::biocCSS()
    template: !expr BiocStyle::biocTemplate()

That way, there is no need to provide an html_document format in BiocStyle.

I don't understand the need to copy code from rmarkdown. If you do copy source code, rmarkdown authors may need to be attributed somehow. Personally I don't care too much about this (CRAN requires it but you are not on CRAN), but since the license is changed to Artistic-2.0 in this package, I'm a little concerned. For example, GPL-3 states

For the developers' and authors' protection, the GPL clearly explains that there is no warranty for this free software. For both users' and authors' sake, the GPL requires that modified versions be marked as changed, so that their problems will not be attributed erroneously to authors of previous versions.

But the documentation ?BiocStyle::html_document mentioned nothing about the differences between BiocStyle::html_document and rmarkdown::html_document. On the contrary, the help pages look so similar, and I'm worried that confusion can be even stronger.

I'm not an expert of different open-source licenses. Just some thoughts on both technical and legal aspects here. I'm by no means saying anything is necessarily wrong or bad. I'm all for open source, and always hope open source projects can succeed.

aoles commented 7 years ago

Hi @yihui, thank you for contacting us and for raising your concerns!

Re the triple colon issue, I admit that it is more of a hack, and I'm fully aware of the downsides of this approach. I will carefully review the code of the package to identify places where ::: can be avoided. However, there are situations in which I think this is the only way to achieve the desired functionality without re-implementing or copying the code from the original package.

For example, just recently I encountered the following problem: the metadata argument to post_knit, pre_processor and post_processor contains raw values from document YAML front matter before they are parsed by knitr. In my use case I have a field containing an R expression, which I would like to access after it has been evaluated. For now I use the approach of re-extracting the metadata from the knitted document with the help of rmarkdown:::partition_yaml_front_matter. Is there is a better solution to this? Or, since this might be a more general issue potentially interesting to other developers too, maybe rmarkdown could provide an interface to access the knitted metadata, what do you think?

When it comes to the second issue: Indeed, it is a bit unfortunate that html_document uses a copy of the original rmarkdown function. In short, this is a remnant from the times when I didn't really know how to retain mathjax functionality with a custom template . Even though I clearly stated my reason in the comment to the function, now I realize I should have mentioned this in the man pages too. But actually, BiocStyle::html_document is a legacy function, as currently we are in the transition phase to the newer BiocStyle::html_document2 format function. (I know: but in this case the name clash with bookdown is completely by chance; read also below) This new function uses a better approach, based on first calling the base function rmarkdown::html_document and only afterwards modifying the template argument in pandoc's call. The codebase of BiocStyle::html_document is going to be replaced by that of BiocStyle::html_document2 soon; I will also work on the documentation to make a clear distinction between our format functions and these provided by rmarkdown.

This brings me to the following: I've noticed that I can use pdf_book from bookdown with a custom base_format. However, there doesn't seem to be a similar interface to html_document-family functions. Right now I need to call bookdown:::html_document_alt to use cross-references and automatic numbering; are there any plans on exporting this mechanism?

Regarding the naming clash: when we started our custom formats we deliberately chose their names to be the same as the corresponding function names from rmarkdown in order to emphasize their close relationship. The original idea was to provide slightly modified functions with the same interface as their rmarkdown counterparts in order to ease up transition to the new style. It never was our intention to confuse the user, nor to claim any authorship of the original functions. Back then it was indeed merely a modified css and template, but these functions started to live their own life and got more and more complex over the years. So, unfortunately, your suggested solution wouldn't work with the current implementation because, for example, we use custom post_processor. But as far as I understand, any format function coming from an external package needs to be referenced by packagename::function in the document front matter. Therefore, there is no ambiguity with rmarkdown's native pdf_document and html_document functions. For regular functions it is clearly a bad idea to have two packages using the same names, but because we use explicit namespacing in calls to format functions this doesn't seem to be a real issue to me here.

Finally, I'm far from being an expert in licensing either, but I agree that maybe it's a good idea to revise our license. Do you have any additional thoughts on this?

Cheers, Andrzej

yihui commented 7 years ago

I have no technical concerns about the naming clashes. As you said, users have to use pkg::function in YAML, so I don't think it is an issue.

If there are any functions you want us to export, please feel free to tell us. For example, if you find html_document_alt useful, you can file a request to bookdown and explain your use case.

I'd appreciate it if you can talk to us before resorting to :::. I found this problem because I was running R CMD check on the reverse dependencies of knitr, and there are ~1500 of them, so this is quite a bit of work for me. Then I found a few packages were broken. After investigation, I found the cause was the use of the internal function pandoc_html_highlight_args. I'm not sure if you understand the overwhelming pressure every time I submit a new version of my packages to CRAN (especially when a package has a lot of reverse dependencies). If the new release breaks any CRAN packages, I'll have to explain it, no matter if it is my fault or not.

If BiocStyle were a CRAN package, it would probably be shot down immediately. I wonder how it could survive on Bioconductor...

aoles commented 7 years ago

Thanks for your quick response! Luckily, BiocStyle is hosted where it is, and I can hack my way around the current limitations of rmarkdowns API ;) But seriously, as promised I will revise the package code to avoid any ::: calls. In case I find any places where there is no other way to achieve the desired result, I will contact you guys as advised.

Actually, I do have some idea of the pressure you are under while maintaining knitr. Around 1/4 af all Bioconductor packages now use BiocStyle, and as I mentioned before we are now transitioning to a new formatting for both PDF and HTML output...

aoles commented 7 years ago

Only now I noticed https://github.com/rstudio/bookdown/commit/f0a30a489f3c8031979dea63e2370bfe8250ca2c; many thanks @yihui for addressing this so quickly by providing the base_format argument to html_document2()!

yihui commented 7 years ago

You are welcome. As I said, if you find anything useful but not exported, just let me know and I'll be happy to consider exporting it.