rstudio / rsconnect

Publish Shiny Applications, RMarkdown Documents, Jupyter Notebooks, Plumber APIs, and more
http://rstudio.github.io/rsconnect/
133 stars 81 forks source link

generate title and name; name is normalized #1023

Open aronatkins opened 11 months ago

aronatkins commented 11 months ago

~When deploying from a directory named "Incredible Shiny Application", the generated name for Posit Connect is Incredible_Shiny_Application and for shinyapps.io is incredible_shiny_application. Both services require a-zA-Z0-9-_ for names, but we lower-case only for shinyapps.io.~

~The name is derived from an incoming title, when available, and otherwise from the content path.~

~When deploying without a title, Connect uses the incoming name to seed the title. This behavior is not changing.~

When deploying from a directory named "Incredible Shiny Application", a generated title mirrors the directory name and name is generated from the title and normalized like incredible_shiny_application.

A generated name is used only when one is not already provided.

Title changes on the server are now reflected in the deployment record.

fixes #1022 related to #1008

aronatkins commented 11 months ago

CI failures due to r-project.org DNS trouble. Will re-run once that clears.

aronatkins commented 11 months ago

Should we do something to generateAppName() to make it less likely to make this mistake again?

I considered modifying generateAppName() but did not want to change its implementation in a breaking fashion, since it is a public function. We disconnected generateAppName() during the build-up to 1.0.0 but didn't mark it as deprecated at that point in time.

Open to suggestions.

hadley commented 11 months ago

Maybe we should deprecate it. It is public, but I doubt many people are using it. There's no evidence of another CRAN package using it: https://github.com/search?q=org%3Acran%20generateAppName&type=code. But maybe it's used by the IDE?

hadley commented 11 months ago

Gah, yes: https://github.com/rstudio/rstudio/blob/ad0e5854c7a0abed5728c476ec3611f458e52d82/src/cpp/session/modules/SessionRSConnect.R#L592-L594

aronatkins commented 11 months ago

The IDE calls generateAppName() to create a name from an incoming title; that makes me want to shift back to using generateAppName() but remove its lower-casing... We could even pass in an (optional) incoming title and act more like the IDE when deployApp() is directly called.

aronatkins commented 11 months ago

@hadley Given https://github.com/rstudio/rsconnect/issues/1022#issuecomment-1785343724, how do we want deployApp() to behave?

We cannot modify generateAppName() because its generated (lower-cased) names are used by the IDE to detect a duplicate deployment.

The best answer would be to use the incoming filename to generate a title alongside the name, which needs less normalization, and then normalize only the name. I considered this option but abandoned it because it felt like too much of a change for what felt like a bug fix.

I'm leaning towards having deployApp() explicitly infer the title.

hadley commented 11 months ago

Yeah, letting deployApp() explicitly infer the title sounds good to me.

aronatkins commented 11 months ago

Yeah, letting deployApp() explicitly infer the title sounds good to me.

OK. I'll take a stab at that and update this PR.