Better preserve facet attributes

[zeileis]

Fixes #208

This is a minimal fix to make sure that facet attributes (especially facet_grid and facet_nrow) are preserved until when they are actually needed. Otherwise they might get lost during split() or rbind() etc.

This is a minimal fix (just adds three lines of code) but not a good/robust fix.

• In tinyplot.default I simply store attributes(facet) early on and then stick all attributes on again before doing the facet layout. Both lines are flagged with a TODO comment.
• In tinyplot.density a very similar approach already existed. However, rather than sticking on all attributes it previously restored only facet_grid but not facet_nrow. So I've only added the latter now because I wasn't sure whether there would be situations when we do not want to restore all attributes.

At the very least, both approaches should be aligned in later iterations of the code, I think. Preferably, though, we would discuss the way how we pass on the facet information more generally. But I guess it makes more sense to reconsider this question when the code has been modularized. Then we will have a better picture of how the facets fit into this.

[vincentarelbundock]

Looks good to me. Should we include a snapshot test?

[grantmcdermott]

Super, thanks @zeileis. Agree that we should revisit once the modularization is finished, but this is a nice workaround in the interim :-)

Please do add a tinysnapshot test or two if you can (one for "hist" and another for "density"). Happy for you or Vincent to merge thereafter.

[grantmcdermott]

Approved but pending snapshot tests.

[zeileis]

Thanks for the feedback. I have now aligned the attribute-handling code between tinyplot.density and tinyplot.default, including the TODO comments in both places and simplifying the density method a bit.

I'm happy to have a look at snapshots but I've never done that so far. Which steps do I need to follow for this?

Also, should I bump the version to 0.2.1.900?

[vincentarelbundock]

For snapshots, the process looks something like this:

Install dependencies:

install.packages(c("tinysnapshot", "tinytest", "svglite"))

Insert snapshot code in one of the test files. Should look something like this:

f = function() tinyplot(Temp ~ Day | Month, data = aq, type = "b")
expect_snapshot_plot(f, label = "aesthetics_type_b")

Run the test suite to create the snapshot file:

library(tinytable)
tinytest::run_test_dir()

Run again to make sure the test passes:

tinytest::run_test_dir()

Commit the snapshot to git and push.

[grantmcdermott]

Also, should I bump the version to 0.2.1.900?

Yes, please. Fwiw I've taken to just using a trailing ".99" to signal dev versions now (i.e. 0.2.1.99), instead of incrementing in the 900s. But fine with either.

[zeileis]

Thank you both! I've bumped the version to 0.2.1.99 now and added the hist/density mtcars examples from #208 as snapshots.

Apologies, Vincent, I just didn't realize that I simply have to run tinytest twice to generate the new snapshot files (and didn't take the time to read the docs). Very elegant - as usual!

I think this should be ready to merge now.

[vincentarelbundock]

Looks great! Thanks for a useful PR.