-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: capitalize first letter of each bullet #409
Conversation
R/update-news.R
Outdated
start_with_pkg <- (grepl(sprintf("^%s ", pkg_name), df$description) || | ||
grepl(sprintf("^%s's", pkg_name), df$description)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"^%s(?: |'s)"
? Extract regex to separate variable? And perhaps combine with regex above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end I realized the "code" regex was useless as toupper()
will not touch those anyway.
R/update-news.R
Outdated
news_item <- split(news_items, seq_len(nrow(news_items))) %>% | ||
purrr::map(capitalize_description) | ||
do.call(rbind, news_item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the need to split here.
map_dfr()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map_dfr()
requires dplyr which fledge does not depend upon hence the awkward split.
Should I define a helper map_dfr
that would hide the split?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I added such a helper)
df$description <- paste0( | ||
toupper(substr(df$description, 1, 1)), | ||
substr(df$description, 2, nchar(df$description)) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprisingly tricky. I was thinking about a rematch2 solution, which could also include the grepl()
logic from above, and allow for vectorization (and more compact code) perhaps?
"Bury the logic in one complex regex..." 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm re-reading ?toupper
and actually one of the examples there is very similar to what I did above (which does not mean there are no better solutions!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One simple solution would be to add a dependency on https://cran.r-project.org/web/packages/snakecase/index.html but that might be overkill for one call only.
Are you still in favor of replacing this with rematch2 or is it fine as is? |
Thanks! No need to dive deeper here. |
Fix #360