Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ggbot #8

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

ggbot #8

wants to merge 3 commits into from

Conversation

jonocarroll
Copy link
Owner

Commands currently supported:

  • change colour of points
  • remove axis, legend, everything

I also aligned the figures in the shortcuts vignette.

aligned shortcuts vignette figures
@jonocarroll
Copy link
Owner Author

I haven't added any tests yet.

Copy link
Collaborator

@AliciaSchep AliciaSchep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool! Really like this idea and this is great start.

#' ggbot("blue points")
ggbot <- function(...) {

tokens <- unlist(strsplit(unlist(rlang::exprs(...)), " "))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With multiple arguments, this mashes all the words together into one vector. Perhaps distinct string inputs should be considered separate commands? e.g. ggbot("rotate x axis", "remove y axis") is something that could supported, and the words in each of those commands would ideally be kept separate. Unless the idea is that there should only one command per ggbot call -- in which case combining all the words together makes sense.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use case I had in mind was either one whole command or a vector of words. The use case you suggest is probably much more likely though; I should iterate over the elements of ... and treat each one as a call to ggbot.

if (length(detect_remove_cmds) > 0 && length(detect_funs) > 0) {

## remove theme
if (detect_funs[1] == "theme") return(theme_void())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if multiple "funs" are provided, maybe all should be removed? e.g. "remove legend x axis" should maybe remove both?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sounds reasonable. I went with just the first because I'm still thinking about contamination between requests, but I need to build some examples with multiple funs and see how that looks.

@jonocarroll
Copy link
Owner Author

This is indeed a first start; if a proper lexer is the better way to go then I'm all for that. It looked like it wasn't too different from a bunch of if statements though. The if blocks could also be farmed out to smaller single-purpose functions for internal tersting.

@AliciaSchep
Copy link
Collaborator

By "start" I just meant we would want to add more commands as more functionality gets added to ggeasy 😄 I think the if based approach is reasonable, and agree that as the functionality expands having smaller helper functions will be nice.

One advantage of if based approach rather than the lexer is that it would probably be more accessible for new contributors to figure out how to add new functionality, so if someone had a new helper they wanted to contribute they could also incorporate it into the bot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants