Fix renderOptions and sorters params, fix documentation, add documentation, add examples

[ghost]

1. Fix to get renderOptions to work. Before, it would be wrapped in a list (even though it was already in a list), and that would cause the caller's argument to not work.
2. The function had a parameter "sorter", but it should actually be "sorters". Before, passing anything to sorter would just get ignored.
3. The docs said that exclusions was a string, but it's really a list. Likewise, it said renederName was a list, but it's really a string. Fixed both of those.
4. Added two examples showing "drilldown" and how to customize c3 graphs (both of these are requested in the bug reports).
5. Added doc about how to use css to prevent pivottable from being clipped in knitr output.
6. Fix cols, rows, and vals documentation to say that they can accept single strings and arrays of strings.

[nicolaskruchten]

Thanks for this PR! Just so I'm clear... these changes do change the current API somewhat, such that code that works with the current version would break once this PR is merged in, right? For example, anyone working around the list-in-a-list issue.

[ghost]

Well...I suppose it would break code where people are doing the 'params$renderOptions = params$renderOptions' hack. But that indeed is a hack that I've only seen mentioned in a couple of bug reports here--it's by no means official API and isn't documented anywhere.

Also, the "params$renderOptions" hack only works if you have one list of renderer options. For example, if you want to pass in c3 AND table renderer options, it would only apply the first one (c3 OR table, but not both). With this PR, it will enable all renderer options that you pass in. So even with the workaround in the past, it was still a bit broken and limited.

And for the "sorter" issue; code from before using a param called "sorter" was simply being ignored. That won't break anything (it will still be ignored), but now it is clear that you should be saying "sorters = …" when you call rpivottable().

Thanks!

[ghost]

FYI, this would close Bug Reports #80, #81, #84, and #92. I show how to do some of the BR requests in the new examples.

[nicolaskruchten]

OK, that sounds promising.

One thing I never really understood about this library (despite now being one of the maintainers) is why/how some parameters are explicitly listed and some are not. For example rendererOptions was not and now you're proposing we "promote" it... ok, but should we explicitly list all the options that are possible to pass into the underlying pivotUI() function? What about new ones like showUI etc?

[nicolaskruchten]

While I'm looking at this, I also notice that the docs list cols, rows and vals as String but in the underlying library at least they are accepted as arrays... Should we fix this also?

[ghost]

Got it, just updated the doxygen comments for cols, rows, and vals. They all work with both single strings and arrays of strings.

IMHO, I like the idea of promoting all of the parameters that we can. It just makes it sooo much easier to know what you can do. Translating how things are done in javascript into R code can be tricky, so the more "baked in" parameters in the R function (especially if I can add examples for them) the better. That was more or less my intention, to experiment with the other pivottable.js features and see if I could add them to the API here (ideally with comments and examples).

[nicolaskruchten]

Cool. Any downsides to promoting all parameters, other than the work required to do it?

[smartinsightsfromdata]

@Blake-Eryx @nicolaskruchten The idea to promote as many parameters as poss. sounds great.

I think there was no clear, deliberate plan in the way we did this at the beginning. Just a set of functionalities, but with the door open to add / specify parameters.

I think it is important leave this door open, but as mentioned it simplifies life to seal with explicit parameters.

I will test @Blake-Eryx PR (thanks!)

@nicolaskruchten as a new change requires Cran submission, do you see worth it to update anything else?

[ghost]

As long as any new parameters have default values (usually just nulls or empty strings), then adding them shouldn't have any backward compatibility issues. And if someone was using an unmentioned feature before (that was silently being handled by "..."), then those will now be directly handled (instead of being sent to "..."). So they will still work.

So, the only downside would be having an exhaustive list of parameters to use. IMO, too many options isn't a bad thing :)

[nicolaskruchten]

I’m fine with promoting all the parameters if it makes things easier! I would recommend an upgrade to the latest version of the pivot table as well for sure, to get the latest fixes, parameters and translations, especially the “showUI” parameter :)

…

On Mon, Oct 15, 2018 at 20:34 Blake Eryx ***@***.***> wrote: As long as any new parameters have default values (usually just nulls or empty strings), then adding them shouldn't have any backward compatibility issues. And if someone was using an unmentioned feature before (that was silently being handled by "..."), then those will now be directly handled (instead of being sent to "..."). So they will still work. So, the only downside would be having an exhaustive list of parameters to use. IMO, too many options isn't a bad thing :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#91 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMbA_r9sIJvJXh-BM8Nt5JtXqCco1S2ks5ulSmtgaJpZM4WWon_> .

[smartinsightsfromdata]

@nicolaskruchten Sounds good. May I propose the following:

1. @nicolaskruchten Updates to the last release in a separate branch
2. @Blake-Eryx applies the PR to the separate branch with the new version (I assume that the there will be no conflicts)
3. We test and if all OK as expected I will prepare the version for CRAN submission (I will add @Blake-Eryx as contributor, of course)

[nicolaskruchten]

Upgrading straight on this branch should be pretty trivial, just a version bump on the appropriate packages.

…

On Sun, Oct 21, 2018 at 09:12 Enzo ***@***.***> wrote: @nicolaskruchten <https://github.com/nicolaskruchten> Sounds good. May I propose the following: 1. @nicolaskruchten <https://github.com/nicolaskruchten> Updates to the last release in a separate branch 2. @Blake-Eryx <https://github.com/Blake-Eryx> applies the PR to the separate branch with the new version (I assume that the there will be no conflicts) 3. We test and if all OK as expected I will prepare the version for CRAN submission (I will add @Blake-Eryx <https://github.com/Blake-Eryx> as contributor, of course) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#91 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMbA35oGJE7iqnUlIePgC4ZI4Q5rnutks5unHLagaJpZM4WWon_> .

[smartinsightsfromdata]

@Blake-Eryx I'm belatedly testing your pr, planning to do a cran submission soon.

good news: the changes pass the R CMD check (mandatory to submit to CRAN).

bad news: drill-down doesn't seem to work (certainly not as in the example from @nicolaskruchten )

Would you be so kind to review the changes?
Moreover, if there are breaking changes, would you be so kind to document them in the doc?
thanks in advance

[jangorecki]

The function had a parameter "sorter", but it should actually be "sorters". Before, passing anything to sorter would just get ignored.

it is resolved by #106