- Notifications
You must be signed in to change notification settings - Fork85
refactor: remove alternate http backends and other deprecated functions#1282
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nealrichardson commentedDec 16, 2025
macos failure is due tohttps://cran.r-project.org/web/packages/xfun/index.html just being released and CRAN must be out of sync somehow |
jonkeane left a comment
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 this. Most of my comments are just linking where we deprecated the thing for the record.
| #' @note These functions only work | ||
| #' for applications deployed to ShinyApps.io. |
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.
Very minor but this can all be on one line now, yeah?
| }else { | ||
| check_bool(quarto,allow_na=TRUE) | ||
| } | ||
| check_bool(quarto,allow_na=TRUE) |
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 this cleanup, FTR#828 is when this deprecation was added
| # Previously exported, but deprecated since 2015 | ||
| authorizedUsers<-function(appDir= getwd()) { | ||
| .Deprecated("showUsers") |
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.
But we still use this internally so we're keeping it around? Is it not possible to shift everything toshowUsers?
And if yes: did the.Deprecated("showUsers") get exposed to users in that internal use? Do we want to maintain that?
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.
Remarkably, yes, it is used internally in one place, and wrapped insuppressWarnings() presumably to squelch the deprecation warning.showUsers looked sufficiently different that I didn't want to swap it in, so instead of deleting this, I just made it internal.
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.
and wrapped in suppressWarnings() presumably to squelch the deprecation warning.
😱 ooph.
ok ok I'm fine with this here, but maybe we should have an issue to clean this up in the future?
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.
Maybe, though it's not clear that this function is problematic per se, maybe it's useful in its internal use?
| if (isStaticFile(appDir)&&!dirExists(appDir)) { | ||
| lifecycle::deprecate_warn( | ||
| when="1.0.0", | ||
| what="deployApp(appDir = 'takes a directory, not a document,')", | ||
| with="deployDoc()" | ||
| ) | ||
| return(deployDoc( | ||
| appDir, | ||
| appName=appName, | ||
| appTitle=appTitle, | ||
| account=account, | ||
| server=server, | ||
| upload=upload, | ||
| recordDir=recordDir, | ||
| launch.browser=launch.browser, | ||
| logLevel=logLevel, | ||
| lint=lint | ||
| )) | ||
| } |
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.
FTR#698 introduced this.
What's the error look like when someone does send a document toappDir? Not that we need to keep that as easy as possible for folks to get on to the right path, but I want to make sure it's notvery hard to decipher.
| lifecycle::deprecate_warn( | ||
| "1.0.0", | ||
| I("The `rsconnect.http` option"), | ||
| details= c( | ||
| "It should no longer be necessary to set this option", | ||
| "If the default http handler doesn't work for you, please file an issue at <https://github.com/rstudio/rsconnect/issues>" | ||
| ) |
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.
FTR:#762
| @@ -1,63 +0,0 @@ | |||
| #' Discover servers automatically | |||
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.
FTR#750
Closes#1281 and then some
I don't see any of these functions referenced inhttps://github.com/search?q=repo%3Arstudio%2Frstudio%20%22rsconnect%3A%3A%22&type=code so I presume this is safe, but maybe that's not enough assurance that nothing here would affect usage in RStudio.