-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-15009: [R] stringr 1.5.0 with the str_like function is already released #15010
Conversation
eitsupi
commented
Dec 17, 2022
•
edited by github-actions
bot
Loading
edited by github-actions
bot
- Closes: [R] stringr 1.5.0 with the str_like function is already released #15009
Signed-off-by: SHIMA Tatsuya <ts1s1andn@gmail.com>
Signed-off-by: SHIMA Tatsuya <ts1s1andn@gmail.com>
Signed-off-by: SHIMA Tatsuya <ts1s1andn@gmail.com>
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
In the case of old issues on JIRA the title also supports:
See also: |
|
|
Signed-off-by: SHIMA Tatsuya <ts1s1andn@gmail.com>
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 small change, otherwise looks good; thanks for making this PR!
do_not_link <- c( | ||
"stringr::str_like" # Still only in the unreleased version | ||
) | ||
do_not_link <- c() |
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.
Given this us now an empty vector, how about we remove this and update the code that uses it?
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.
That would be fine, but considering the possibility of similar cases occurring in the future, I thought it was worth leaving it for then.
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.
That's logical. The reason I suggested removing it is that these cases occur very infrequently if at all; this is the only example out of >200 functions and happened because we implemented a function from the dev version of stringr, which is highly unusual and there are no plans to do this again.
That said it's so minor, I'm not going to block merging because of it; let me know if you'd rather leave it in and I'll approve this PR.
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.
Thank you for the explanation. I don't know how str_like
was implemented here, so your observation that the same is unlikely to happen in the future makes sense to me.
However, updating the script that generates the documentation seems generally more compelling to track in a separate PR, given that PR commits are squashed, so I would still prefer to keep it in this PR. (I think it would be far easier to revert from the future if this process were removed in another PR)
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.
That's reasonable, makes sense.
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.
Thanks for making this update!
Benchmark runs are scheduled for baseline = 2c768a1 and contender = 6d3aec4. 6d3aec4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |