Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Reduce the usage of appPath #696

Merged
merged 4 commits into from
Feb 23, 2023
Merged

Reduce the usage of appPath #696

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b208f6
Reduce the usage of appPath
hadley Feb 22, 2023
3987bad
Document changes to hook argument
hadley Feb 23, 2023
fc7eb28
Rename argument in deploymentTarget()
hadley Feb 23, 2023
cf92aad
Document what I've learned about recordDir
hadley Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions R/deployApp.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ deployApp <- function(appDir = getwd(),
}

# invoke pre-deploy hook if we have one
runDeploymentHook(appPath, "rsconnect.pre.deploy", verbose = verbose)
runDeploymentHook(appDir, "rsconnect.pre.deploy", verbose = verbose)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a functional change to package contract, correct? Given a target Rmd in a directory containing lots of Rmd, how will the hook know what you're about to deploy?

This work initially landed with #283 and #295, but without a specific motivating example.

Additionally, I think the IDE deploy pane transfers many (most? all?) R options and environment variables from the R session to the deploy pane (without using these hooks).

I did find one issue referencing this hook:

and a reference from thematic!!:

Unfortunately, I don't know of many in-the-wild uses of this feature that might care about the file-path vs. directory change. At the very least, we should NEWS this adjustment and mark it as a potentially breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a change, and the documentation for the hooks is murky:

\item{\code{rsconnect.pre.deploy}}{A function to run prior to deploying content; it receives as an argument the path to the content that's about to be deployed.}

What does "content about to be deployed" mean? (especially if you've used appFiles or friends).

My guess is that this won't affect any code in the wild, but I'll add a news bullet.

FWIW I think the thematic usage would break when appPath is not a directory.


appFiles <- standardizeAppFiles(appDir, appFiles, appFileManifest)

Expand All @@ -247,7 +247,7 @@ deployApp <- function(appDir = getwd(),
}

# determine the deployment target and target account info
target <- deploymentTarget(appPath, appName, appTitle, appId, account, server)
target <- deploymentTarget(recordDir, appName, appTitle, appId, account, server)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internally, deploymentTarget calls this argument appPath; should it be renamed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed it to recordPath, in anticipation of renaming recordDir to recordPath in a future PR.


# test for compatibility between account type and publish intent
if (!isCloudServer(target$server) && identical(upload, FALSE)) {
Expand Down Expand Up @@ -353,7 +353,7 @@ deployApp <- function(appDir = getwd(),

# invoke post-deploy hook if we have one
if (deploymentSucceeded) {
runDeploymentHook(appPath, "rsconnect.post.deploy", verbose = verbose)
runDeploymentHook(appDir, "rsconnect.post.deploy", verbose = verbose)
}

logger("Deployment log finished")
Expand All @@ -378,7 +378,7 @@ needsVisibilityChange <- function(server, application, appVisibility = NULL) {
cur != appVisibility
}

runDeploymentHook <- function(appPath, option, verbose = FALSE) {
runDeploymentHook <- function(appDir, option, verbose = FALSE) {
hook <- getOption(option)
if (!is.function(hook)) {
return()
Expand All @@ -387,7 +387,7 @@ runDeploymentHook <- function(appPath, option, verbose = FALSE) {
if (verbose) {
cat("Invoking `", option, "` hook\n", sep = "")
}
hook(appPath)
hook(appDir)
}


Expand Down