Centralized and simplify xlab and ylab processing

[vincentarelbundock]

The xlab and ylab arguments get processed at several different places in the (already very long) tinyplot() function. This makes it very difficult to follow, audit, or improve the logic.

This is an attempt to pull out all the processing inside a single helper function called sanitize_xylab(). This allows a few improvements for consistency:

1. tinyplot_add() the deparsed label from the last plot is printed. test-misc.R
2. tinyplot(0:10) prints deparsed "0:10" on both axes instead of "Index". test-type-other.R
3. more consistent use of deparsed names (ex: ribbon)

[vincentarelbundock]

@grantmcdermott this is an internal refactoring for maintainability.

The only change to snapshots you'll notice is that when we supply only x but not y and tinyplot sets y=x automatically, then we print the label on both axes instead of one label and one Index. That seems more descriptive, and it's a corner case anyway, so this is an inconsequential change from the users' perspective.

This is a self-contained PR. I plan to take a more serious look at tinyplot.R when I have time, because it seems a bit out of control in terms of length and complexity. But I'll try to do it in small chunks for easier review.

[grantmcdermott]

The only change to snapshots you'll notice is that when we supply only x but not y and tinyplot sets y=x automatically, then we print the label on both axes instead of one label and one Index. That seems more descriptive, and it's a corner case anyway, so this is an inconsequential change from the users' perspective.

Hmmm, this is actually something that I'd like to preserve the current behaviour for. I agree it's kind of a corner case, but it's one I use surprisingly often myself and is often the first thing I show audiences when comparing tinyplot to regular base plot, where my goal is to emphasise that you get the exact same end result.

[grantmcdermott]

But I agree that the current, nested if-else deparsing logic is convoluted and too finnicky. This is also something that @zeileis has brought up a few times, which has caused headaches downstream (iirc #423 is one such case). So I'm all for improving and simplifying where possible.

[vincentarelbundock]

is often the first thing I show audiences when comparing tinyplot to regular base plot

Somehow I had not realized this was base behavior. Makes sense to stick with it then.

All the old tests now pass and I added no new snapshots.

[grantmcdermott]

Looks good, thanks. Just one or two minor comments.

[grantmcdermott]

Should we just do list2env(sanitize_axes(...), environment()) directly to avoid writing the temporary list?

[grantmcdermott]

Same comment as above; I think it's safer to do this directly than creating a temporary variable in case something goes wrong and the value of tmp doesn't get (re)assigned properly.

[vincentarelbundock]

made requested changes and merged. Thanks for the review!

[zeileis]

Vincent, thanks for this. This is much clearer than before.

Given that you thought about this, I wanted to ask you whether you had any ideas for further improvement. When writing new type_* functions, I often struggle with the xlab/ylab defaults because the sanitizers are run first before type_data() can modify anything.

One alternative approach would be that the type_*() could specify somehow what type of sanitizing is desired (e.g., whether it's a frequency, range, index, etc.).

Another possibility would be to explicitly list all known types from within the package in sanitize_xylab() and only process these. Thus, the default would be that no defaults are sanitized for new/unknown type_*(). If the sanitize_xylab() function were exported, then a new type_*() function could also later on employ the same sanitization as a known type.

Maybe you thought about this type of problem and have some ideas?

[vincentarelbundock]

That's a good question, and I hadn't really thought about it.

At the moment, I think I would lean towards this:

Another possibility would be to explicitly list all known types from within the package in sanitize_xylab() and only process these.

It's not the most elegant, and I'm sure there's a fancy approach we could invent, but I feel the main problem in readability beforehand is that things were done at several different points, and that made the logic very opaque. Keeping everything in one place is much simpler, and facilitates re-use.

If the sanitize_xylab() function were exported, then a new type_*() function could also later on employ the same sanitization as a known type.

Not sure we need to pollute the namespace for something like this, since the number of users affected is likely small. But one thing we could easily do is pass the deparsed x_dep, y_dep, and friends to type_*() functions (as we do many other parameters).

The actual sanitation only ever takes a couple lines anyway, so custom type functions don't actually need a helper for this.

[zeileis]

Yes, maybe that would be an improvement. The price would be, of course, that every time we introduce a new type_*() we have to touch code outside of the type_*(). But hopefully it would be easier to write a first draft that does not necessitate touching anything outside...