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.

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

Adjust to new RStudio version numbering #2410

Merged
merged 8 commits into from
Feb 23, 2022
Merged

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Feb 22, 2022

Closes #2397

As per @jgutman advice:

  • Use the long version number for RStudio, when available. Fall back to shorter version otherwise.
  • URL encode the version number.
  • Add manual=true to the URL we use when checking for updates.

Also makes this helper much less likely to throw an error, even if it does not succeed.

R/sitrep.R Outdated Show resolved Hide resolved
R/sitrep.R Show resolved Hide resolved
@jennybc
Copy link
Member Author

jennybc commented Feb 22, 2022

Would appreciate a quick 👀 @romainfrancois and/or @jgutman (I can't officially request a review from you).

Do you foresee any negative consequences of this?

R/sitrep.R Show resolved Hide resolved
R/sitrep.R Outdated
Comment on lines 26 to 27
# I'll take silent failure here over dev_sitrep() falling over completely
# if this download fails
Copy link
Member Author

Choose a reason for hiding this comment

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

This captures why I've made this function a bit more robust. In general, I think "sitrep" functions should basically never throw an error, because you're often using them to bring clarity to weird, sub-optimal situations.

@jennybc
Copy link
Member Author

jennybc commented Feb 22, 2022

@jgutman can you give me concrete examples of the different version number forms I need to handle?

R/sitrep.R Show resolved Hide resolved
@jennybc
Copy link
Member Author

jennybc commented Feb 22, 2022

The quoting here is a surprise:

$version
[1] ‘1.4.1717’

@jennybc
Copy link
Member Author

jennybc commented Feb 22, 2022

It looks like rstudioapi::versionInfo()$version is an instance of c("package_version", "numeric_version"), whereas $long_version is a simple string. So I guess it needs an as.character().

@jennybc
Copy link
Member Author

jennybc commented Feb 22, 2022

I'm under the impression that I can't / really shouldn't use RStudio.Version() because it's made available in an RStudio context via some sort of shim, whereas it's much safer to always go through rstudioapi.

@jgutman
Copy link

jgutman commented Feb 22, 2022

@jgutman can you give me concrete examples of the different version number forms I need to handle?

An older version
https://dailies.rstudio.com/rstudio/juliet-rose/desktop/macos/
Returns $version [1] ‘1.4.1717’ but no long_version available

A newer release version
https://dailies.rstudio.com/version/2022.02.0+443/
$version
[1] ‘2022.2.0.444’
$long_version
[1] "2022.02.0+444"

A preview version
https://dailies.rstudio.com/version/2022.02.0-preview+391/
$version
[1] ‘2022.2.0.391’
$long_version
[1] "2022.02.0-preview+391"

A daily version
https://dailies.rstudio.com/version/2021.09.0-daily+328/
$version
[1] ‘2021.9.0.328’
$long_version
[1] "2021.09.0-daily+328"

@jgutman
Copy link

jgutman commented Feb 22, 2022

I'm under the impression that I can't / really shouldn't use RStudio.Version() because it's made available in an RStudio context via some sort of shim, whereas it's much safer to always go through rstudioapi.

Probably right, rstudioapi::versionInfo() will be equivalent!

@jennybc
Copy link
Member Author

jennybc commented Feb 22, 2022

Here's how the IDE does this:

https://github.com/rstudio/rstudio/blob/main/src/cpp/session/modules/SessionUpdates.R

It gets called with manual = TRUE, secure = TRUE. Or at least that seems to be a safe assumption for me, in terms of updating here.

Copy link

@jgutman jgutman left a comment

Choose a reason for hiding this comment

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

LGTM

@jennybc
Copy link
Member Author

jennybc commented Feb 22, 2022

OK this was more than I bargained for, but I think it's done.

In the future, this gives us a better chance of detecting if this function has stopped working again and shows the version number it's falling over for.
@jennybc jennybc merged commit 93072ca into main Feb 23, 2022
@jennybc jennybc deleted the rstudio-new-version-numbers branch February 23, 2022 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

devtools::dev_sitrep() fails after the change in RStudio version numbering
2 participants