fix getMode() fallback for old rstudio versions#327
Merged
Conversation
On very old versions of RStudio that lack the internal .rs.isDesktop() helper, the getMode() fallback path threw 'attempt to apply non-function'. Guard the .rs.isDesktop() call and fall back to versionInfo()$mode (available since RStudio 0.97.124) when it is absent. The .rs.isDesktop() check is retained as the preferred fallback for performance reasons (#280): versionInfo() is slow because it reads the citation file. Fixes #326.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #326.
Problem
On very old versions of RStudio,
getMode()fails withattempt to apply non-function. Neither path works:hasFun("getMode")isFALSE(the API function is recent), so the fallback runs.rstudio$.rs.isDesktop(), but.rs.isDesktopdoes not exist in that old version, so it resolves toNULLand calling it errors.Why
.rs.isDesktop()is in the fallbackgetMode()was added in #280 as a fast shortcut forversionInfo()$mode.versionInfo()is slow because it reads the citation file (readCitationFile()), and the benchmark in that issue showed the internal call at ~410ns vsversionInfo()$modeat ~386us..rs.isDesktop()was chosen specifically to avoid that cost, so simply replacing the fallback withversionInfo()$modewould reintroduce the slowness #280 fixed.Fix
Make the fallback three-tier so it honors the performance intent and works on old versions:
getMode-- fast path on modern RStudio (unchanged)..rs.isDesktop()-- retained, but guarded withis.function()so a missing helper no longer throws.versionInfo()$mode-- new final fallback for ancient RStudio that lacks both.$modehas been available since RStudio 0.97.124.The slow
versionInfo()path only triggers on RStudio old enough to lack both the API function and.rs.isDesktop(), so callers like cli stay on the fast path on current RStudio.Also switched
as.environment("tools:rstudio")to the existingtoolsEnv()helper, for consistency with the rest of the file and to make the path mockable.Tests
Added
tests/testthat/test-code.Rcovering both the.rs.isDesktop()path (and assertingversionInfo()is not called there) and theversionInfo()fallback when the helper is absent.devtools::test(): 20 pass, 0 fail (the 1 skip is the pre-existing RStudio-only document-API test).