Apply guide how to use {ggplot2} in packages about mlr3viz HOT 10 CLOSED

What exactly should be changed?

from mlr3viz.

What exactly should be changed?

Haven't checked in detail but the person being assigned should do this.
Maybe there is not much to change because the functions are fairly limited in allowing top-level arguments to be passed down.

But at least we should use {vdiffr} for checking the outputs - as already mentioned in #6.

(There would be much more potential for this in {mlr} 😄 )

from mlr3viz.

I skimmed through the suggestions made by the devs and our code. We could apply the following:

• They discourage to import ggplot2 completely with #' @import ggplot2.
• They promote vdiffr for checking the outputs.

The other points are covered by our code.

from mlr3viz.

Are we using the .data[[var]] notation already and create the geoms/mappings in front of the actual ggplot call?

from mlr3viz.

Are we using the .data[[var]] notation already

Okay I thought this does not apply to us because we are using aes_string(). However, it seems that aes_string will be deprecated in a future version.

from mlr3viz.

Okay I thought this does not apply to us because we are using aes_string(). However, it seems that aes_string will be deprecated in a future version.

We can also wait until this is really deprecated, and then see what we really need to switch to. This was already changed so many times...

Also, I'm not sure about vdiffr. It seems to only work on travis+appveyor, and seems to be broken for github actions. If you want to have it, please make it really optional (don't put into suggests, skip test if not installed).

from mlr3viz.

We can also wait until this is really deprecated, and then see what we really need to switch to. This was already changed so many times...

Well, they decided to pay a person to write a guide for it now. Also the .data[[x]] notation is rlang based and around for some time and will probably stick.
I do not really get why we should way until something goes away completely.
New PRs will adapt the current structure and by enforcing it now we reduce work in the future.

Also, I'm not sure about vdiffr. It seems to only work on travis+appveyor, and seems to be broken for github actions. If you want to have it, please make it really optional (don't put into suggests, skip test if not installed).

Not really mature, I agree. Nevertheless I find the approach very promising and we can conditionally use it in the tests and enable it completely once it becomes stable.

It's skipped on CRAN anyway but very helpful for local testing.

from mlr3viz.

I think it does not make sense to put effort in vdiffr:
We do not develop new plot methods. We just use ggplot2 in a pretty basic way. As long as the correct data goes in the correct plot should come out. That is a promise we should rely on without having to test it. We would just test whether ggplot2 works correctly but this should not be our problem.

What you can conclude is, that we should test that the input is correct.

from mlr3viz.

i am also against visual unit tests. this was a huge (!) pain-in-the-ass before. we wasted so many hours on getting this done, then had to invest even more after internal changes in ggplot2.

and: i am not sure we EVER got any benefits from it - in contrast to our normal unit tests.
so as long someone with longer experience with this can convincingly argue why this is great and how it should be done in detail, i think we shouldnt do it.

also: the issue here is pretty unconcrete. can we pls convert it into something thats actionable? or close?

from mlr3viz.

Where did we ever have such tests before? This was not yet possible before?

Ok, not going to argue further about the advantages then if so many people do not want it/like it.

from mlr3viz.