-
Notifications
You must be signed in to change notification settings - Fork 70
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
Fix #621 #733
Fix #621 #733
Conversation
This commit conditions on the presence of rstudioapi 0.17.0. If satisfied then use the new getMode() function (https://github.com/rstudio/rstudioapi/releases/tag/v0.17.0). Otherwise maintain the old behaviour. Printing of a slowish data frame in RStudio improved from ~18 seconds, to ~3 seconds with this commit and rstudioapi v0.17.0.
R/aab-rstudio-detect.R
Outdated
# Pull version from namespace rather than via `utils::packageVersion()` | ||
# to avoid slowdown (see https://github.com/r-lib/rlang/pull/1657 and | ||
# https://github.com/r-lib/rlang/issues/1422) | ||
new_api <- ns[[".__NAMESPACE__."]][["spec"]][["version"]] >= "0.17.0" |
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.
You are comparing version numbers as strings, you probably want package_version()
? Also, getNamespaceVersion()
is simpler and better IMO.
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.
Yes to both! Thank you. Have updated the PR (performance improvement remains the same for by use case).
FI - I cribbed that comparison from https://github.com/r-lib/rlang/blob/3d48c13d052724cd5997a730ea41bc8e6991691b/R/standalone-cli.R#L463. Is it worth flagging the string comparison issue there as well?
Thanks! Please see the comment above. |
- fixes a comparison between strings to a comparison between a numeric_version and a string (a valid op). - Uses getNamespaceVersion as clearer and part of base API as well as still being performant.
Thanks! ❯ getNamespaceVersion(asNamespace("rstudioapi"))
version
"0.16.0"
❯ getNamespaceVersion(asNamespace("rstudioapi")) > "0.16-1"
version
TRUE
❯ package_version(getNamespaceVersion(asNamespace("rstudioapi"))) > "0.16-1"
[1] FALSE |
Cheers for your patience. I'm a doughnut! (I was sure I'd checked it returned a |
Thank you! |
This commit conditions on the presence of rstudioapi 0.17.0. If satisfied then use the new getMode() function
(https://github.com/rstudio/rstudioapi/releases/tag/v0.17.0). Otherwise maintain the old behaviour.
Printing of a slowish
data frametibble in RStudio improved from ~18 seconds, to ~3 seconds with this commit and rstudioapi v0.17.0.