Rapporter / pander

An R Pandoc Writer: Convert arbitrary R objects into markdown
http://rapporter.github.io/pander/
Open Software License 3.0
294 stars 66 forks source link

row.names and col.names argument for pandoc.table() #282

Closed jranke closed 7 years ago

jranke commented 7 years ago

I think these arguments are important, therefore I propose to place them early in the argument list. This is a matter of opinion of course. I tried to use match.arg on these, but it throws an error. Maybe the arguments need to be renamed for this to work, but I think it's good to be compatible to knitr::kable() regarding the naming of the arguments.

I also tried to place the code later in the function definition of pandoc.table, where the other arguments are initialized, but then it fails when row.names is set to FALSE, so it seems some code uses the row names before that place.

Closes #281.

codecov-io commented 7 years ago

Current coverage is 79.58% (diff: 100%)

Merging #282 into master will increase coverage by 0.04%

@@             master       #282   diff @@
==========================================
  Files            12         12          
  Lines          3602       3615    +13   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2865       2877    +12   
- Misses          737        738     +1   
  Partials          0          0          

Powered by Codecov. Last update 2c1b479...dc9a25e

daroczig commented 7 years ago

First of all, thanks a lot for this!

I think you are updating the row/col names too early -- before some important object updates. Think about eg table or multi-dimensional table objects, which are converted into standard data.frames. So I'd update the row and col names right after the start of the "(re)set row and column names" section. Did you try that?

Also, regarding param names: that's good, but I'd place those at a later point (eg before emphasize.rownames) not to break already existing code relying on the position of args.

jranke commented 7 years ago

I moved the arguments to the position just before emphasize.rownames.

Yes, I tried to move the code handling row.names and col.names to a later position. If I put it below

if (is.null(mc$justify))

I get errors.

jranke commented 7 years ago

I think I have addressed your points with the last two commits, thanks for your feedback!

jranke commented 7 years ago

@daroczig Hi! Is there anything else I can do to convince that this PR improves pandoc.table?

daroczig commented 7 years ago

Sorry for the long delay with this, thanks a lot! :+1:

jranke commented 7 years ago

No Problem, glad to see that it's merged now!

Am 1. Juli 2017 02:11:16 MESZ schrieb "Gergely Daróczi" notifications@github.com:

Sorry for the long delay with this, thanks a lot! :+1:

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/Rapporter/pander/pull/282#issuecomment-312397377

-- Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.