pandoc.table.return refactoring

[RomanTsegelskyi]

I have started working on refactoring pandoc.table.return through converting to intermediate representation. As for now I am trying data.frame as an intermediate representation. IMHO that should be a great simplification of many things and I will be pushing new commit as I refactor and test the other parts.

[daroczig]

Let me know if you need any help here. What does empC stand for?

[daroczig]

Minor things to be addressed:

• L904 seems like a hard-coded value introduced in 135cb28. This should be panderOptions("table.split.cells"), nope?
• L936 is also an extremely old issue described in more details at split.table is not working #164. Can you please figure this out and make some test cases for the issue? Maybe we have that extra 4 on purpose, not sure :( I will try to dig in git history, but please do the same and/or think about/test what happens by fixing split.table is not working #164.
• The rmarkdown pipe-delimited table has a bug, maybe we should open a new ticket, that it does escapes pipes (e.g. a pipe in a cell). Sorry for adding this here, just not to forget about -- totally unrelated, but I've just realized that re-reading all lines of the function :)
• There's many one-liner if and for expressions without the curly braces, which are not tracked in covr AFAIK. We should always use the full syntax. Can you please do this update?

These are not high priority, but probably we should do this in the means of refactoring, so that we can finish thinking about this in the near future, and can concentrate on new features, or rather documenting existing ones :)

Other than these minor things, I see nothing serious to be refactored. I thought that converting all objects to the intermediary would suppress more part of the function, but what we have here is still extremely important, as makes maintenance a lot easier in the future.

[RomanTsegelskyi]

@daroczig, I added braces to if/for and fixed hardcoded values. Do you think it's reasonable to add braces in all other places to have more correct coverage?

Also, I don't quite understand the issue with rmarkdown. Can you please explain more or give an example?

[daroczig]

Thanks a lot!

Re curly braces: yeah, it seems that it's required for covr :(

Re rmarkdown, please see this example:

> pander(data.frame(a = 'foo|bar', b = 'my missing cell'), style = 'rmarkdown')

|    a    |        b        |
|:-------:|:---------------:|
| foo|bar | my missing cell |

Rendering as:

a	b
foo	bar

[RomanTsegelskyi]

@daroczig, I was trying to find out about extra 4 space described in #164, and it seems that it has been there from the moment the feature of splitting tables was added (4c1d7d5). Based on my search after addition it was not changed (I might be mistaken though).

Now as to why rationale behind it, IMHO it was done to automatically account for separators like | and spaces surrounding them. Based on pandoc.table in that commit, there was only 1 style something similar to what is now called grid. And for grid the split.tables works correctly.

> pander(data.frame(a = '7 chars', b = paste(rep('Δ', 5), collapse = ' ')), split.table = 23, style='grid')


+---------+
|    a    |
+=========+
| 7 chars |
+---------+

Table: Table continues below



+-----------+
|     b     |
+===========+
| Δ Δ Δ Δ Δ |
+-----------+

> pander(data.frame(a = '7 chars', b = paste(rep('Δ', 5), collapse = ' ')), split.table = 24, style='grid')


+---------+-----------+
|    a    |     b     |
+=========+===========+
| 7 chars | Δ Δ Δ Δ Δ |
+---------+-----------+

> nchar("| 7 chars | Δ Δ Δ Δ Δ |", type='width')
[1] 23

So it seems to me that addition of spaces need to be adjusted accourding to style. What do you think?

[daroczig]

Kudos for the great debugging!
And yeah, it rings a bell now.

Your suggestion makes sense for sure, the number of extra spaces should be determined by the separator/style. Thanks again.

[daroczig]

👍