moodymudskipper / nakedpipe

Pipe Into a Sequence of Calls Without Repeating the Pipe Symbol.
69 stars 7 forks source link

addin to translate nakedpipe to 'regular' magrittr? #9

Closed daranzolin closed 4 years ago

daranzolin commented 4 years ago

I spent the day with nakedpipe and really enjoyed it! However, I work in an office with relative newcomers to R and this is probably a tad exotic for them. But having an RStudio add-in that 'translates' nakedpipe code to 'regular' magrittr code would let me enjoy nakedpipe while not stifling collaboration.

moodymudskipper commented 4 years ago

Thanks for giving it a chance! I like your suggestion. I think I can do it with string manipulation only, no need to parse the call. The addin would work on all pipe variants, we d just lose the debugging or logging feature after convertion. I can either deal with side effects by using %T>%, on {}expressions, which won't be 100% robust (e.g. Assignments). Or remove side effects altogether when converting. Or fail with message saying side effects are not supported for conversion to magrittr (easiest and safest).

daranzolin commented 4 years ago

Not my call I guess, but I think failing with the message that side effects are not supported makes the most sense (and is happily the easiest and safest).

moodymudskipper commented 4 years ago

We could have recursive pipe calls. Just check if any nakedpipe symbol exists in the {} expression and fail if we find one. User can still convert in several steps from the inside out in those rare cases.

moodymudskipper commented 4 years ago

This is working quite well so far, I've pushed what I did already.

I have 2 addins, so we can convert both ways, you just have to select the call and trigger them and it will and your code will be modified.

I've assigned them to Alt+M and Alt+N for conversion respectively to magrittr and nakedpipe. and using these I can toggle seamlessly between the following pairs :

# standard chain
cars %>%
  head(2) %>%
  dim()

cars %.% {
  head(2)
  dim()
}
#assignment pipes
cars %<>%
  head(2) %>%
  dim()

cars %<.% {
  head(2)
  dim()
}
# selection starting with assignment
# AND "with"/"$" feature
test <- cars %>%
  head(2) %$%
  mean(dist)

test <- cars %.% {
  head(2)
  with(mean(dist))
}
# transformation of + so we can convert pipe chains including ggplot calls
cars %>%
  head() %>%
  ggplot(aes(speed, dist)) +
  geom_point()

cars %.% {
  head()
  ggplot(aes(speed, dist))
  +geom_point()
}

It doesn't support nested calls of either nakedpipe or magrittr, but I don't think it's worth it.

Since I went all this way, I think I should support side effects too, try to support the conversion as well as possible, error if it's for sure an unsupported feature by magrittr, and warn otherwise in any case as nakedpipe and magrittr work with different environments so side effects are always dangerous.

Thank you for the idea @daranzolin, I think it's great for the appeal of the package because many users will be like you, using this fringe package that their coworkers don't know, and this feature is reassuring.

If a user uses both packages they might want to turn a regular pipe chain into a naked pipe call after a few pipes too, I've done it manually before.

I hope you get the opportunity to test it a bit for me.

Also, it seems to me that the package will be ready for CRAN after this feature and https://github.com/moodymudskipper/nakedpipe/issues/17 . Do you agree ? Do you have more comments ?

to do :

moodymudskipper commented 4 years ago

I finally used a single addin for both conversions.

Here's the result :

nakedpipe_addin

daranzolin commented 4 years ago

I used the new addin with a designated hot key and it works seamlessly. Fantastic tool. The indentation is a little different than my personal tastes, but I will be using this as of today. I may even demo it in an Rmd template review meeting tomorrow. Thanks for making it happen!

As for CRAN readiness, I don't have an informed opinion (my previous few submissions were rejected for various reasons lol), but this feels like a pretty robust package already.

moodymudskipper commented 4 years ago

The addin is documented and works with all current features except for side effect assignments, which I think cannot work with magrittr unless I build a very weird translations. So I think I can close this

github-actions[bot] commented 2 years ago

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.