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

use_msrv() #384

Merged
merged 18 commits into from
Sep 7, 2024
Merged

use_msrv() #384

merged 18 commits into from
Sep 7, 2024

Conversation

kbvernon
Copy link
Contributor

@kbvernon kbvernon commented Sep 7, 2024

This enables setting MSRV in DESCRIPTION SystemRequirements from R.

It is part of the push for a CRAN release: #362.

Copy link
Contributor

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

As discussed on discord, I think there could be more details on the relationship between the MSRV and the CRAN version. Here are a couple of comments, I hope it helps

R/use_msrv.R Outdated Show resolved Hide resolved
R/use_msrv.R Outdated Show resolved Hide resolved
R/use_msrv.R Outdated Show resolved Hide resolved
R/use_msrv.R Outdated Show resolved Hide resolved
R/use_msrv.R Outdated Show resolved Hide resolved
R/use_msrv.R Outdated Show resolved Hide resolved
R/use_msrv.R Outdated Show resolved Hide resolved
R/use_msrv.R Outdated Show resolved Hide resolved
R/use_msrv.R Outdated Show resolved Hide resolved
Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
R/use_msrv.R Outdated Show resolved Hide resolved
kbvernon and others added 10 commits September 7, 2024 11:11
Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
@JosiahParry
Copy link
Contributor

JosiahParry commented Sep 7, 2024

I think the last thing that this needs is a snapshot test. Do you think you can do that?

For example the test would be created using usethis::use_test("use_msvr")

test_that("use_msrv() modifies the MSRV in the DESCRIPTION", {
  skip_if_not_installed("usethis")

  path <- local_package("testpkg")
  # capture setup messages
  withr::local_options(usethis.quiet = FALSE)
  use_extendr(path, quiet = TRUE)
  use_msrv("1.70")

  expect_snapshot(cat_file("DESCRIPTION"))
})

@JosiahParry JosiahParry enabled auto-merge (squash) September 7, 2024 20:23
@JosiahParry
Copy link
Contributor

Assuming the linter is finally satisfied this is ready to be merged!

@JosiahParry JosiahParry merged commit 849e09b into extendr:main Sep 7, 2024
17 checks passed
@kbvernon kbvernon deleted the use_msrv branch September 7, 2024 20:51
@etiennebacher
Copy link
Contributor

@JosiahParry Coming a bit late after this got merged but could you include the table you published on Mastodon about the R version and platforms on CRAN and the associated Rust version? I think it would fit either in the doc of this function or in the vignette about CRAN

@JosiahParry
Copy link
Contributor

I think the problem with that is that we don't know when the rust versions will change and it may be that the table is out of date already. I did make an issue for the extendr website to report the CRAN versions which I think would be a more appropriate place for them extendr/extendr.github.io#40

@etiennebacher
Copy link
Contributor

Yes, I see the problem of this getting outdated. It could be that a simple CI script runs weekly to update it. But anyway, I just wanted to be sure this was tracked somewhere :)

@JosiahParry
Copy link
Contributor

Yes, I see the problem of this getting outdated. It could be that a simple CI script runs weekly to update it. But anyway, I just wanted to be sure this was tracked somewhere :)

We ought to add a CRAN Support section in the user guide where this could be reported. It could be a better version of https://extendr.github.io/rextendr/dev/articles/cran-compliance.html

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.

3 participants