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

Change maintainer for resubmission as 0.3.9 #104

Merged
merged 12 commits into from
Jul 26, 2021
Merged

Change maintainer for resubmission as 0.3.9 #104

merged 12 commits into from
Jul 26, 2021

Conversation

kuriwaki
Copy link
Member

Mostly clearing out some things that we don't seem to be using.

As for the concern with readr 2.0.0 (OuhscBbmc/REDCapR#343), I ran the local tests after updating 2.0.0 and I don't see any issues. After rethinking I don't think we need to set >= 2.0.0 in DESCRIPTION -- the modal dataverse file is a TSV, and our default readr::read_tsv should work regardless of v2 or v1. @wibeasley does that sound right?

for-developers/developer-tasks.R Show resolved Hide resolved
Makefile Show resolved Hide resolved
for-developers/notes-datafest-20201.md Show resolved Hide resolved
@kuriwaki, when I was working on REDCapR today, I see that the following code mimics R-hub setting.  I still would submit to R-hub before submitting to CRAN.  But this should catch some things locally so you don't have to wait for R-hub to finish on ~3 different OS builds.

https://blog.r-hub.io/2020/12/01/url-checks/
@wibeasley
Copy link
Contributor

@kuriwaki, agreed. If it doesn't seem to matter, don't add it as a requirement. But I'd consider adding the requirement after a reasonable time for everyone to update. Say, a year?

This is because we'll always need keep this dichotomy in mind until there's a single parser engine. If there's a weird bug that sometimes works and sometimes fails, remember that one user's machine may be running the old readr engine, while another's is running the new readr/vroom engine.

@kuriwaki, here's a newish package that saved me a lot of time.  I think with R 4.1, they're being pickier with URLs, particularly those being redirected from http to https.  This one detect them (and corrects them, if you'd like) without running the whole R-hub (or that R-hub equivalent I included earlier today).

https://github.com/r-lib/urlchecker
@kuriwaki
Copy link
Member Author

Sounds good about clarification in the long term.

Though actually, our win-devel gh actions check is consistently failing and the error logs do suggest it might be readr 2.0.0 related.

@wibeasley
Copy link
Contributor

Does the win-builder error look like the GitHub Actions error?
https://github.com/IQSS/dataverse-client-r/runs/3129414663?check_suite_focus=true#step:8:3202

x Failed to build source package 'readr'

If so, I'm not sure you can do much, at least tonight. It looks like it's correctly retrieving readr 2.0, but I guess not building it. This is a few steps before anything to do with the dataverse package.

https://github.com/IQSS/dataverse-client-r/runs/3129414663?check_suite_focus=true#step:8:62

I bet it's some CRAN binary problem that will be worked out by tomorrow.

I had a smooth submission with REDCapR tonight, which also has a the readr 2.0 prereq. So that's a good sign for this.

If you want me to look at the win-builder error, you can post the url. It stays on their server for a day or two.

@kuriwaki
Copy link
Member Author

Ok good to hear about your RedcapR success. I was referring to only the githb Actions error, but I did run devtools::check_win_devel() after your note and got an error with the vignette:

Error in (function (command = NULL, args = character(), error_on_status = TRUE,  : 
  System command 'R' failed, exit status: 1, stdout + stderr (last 10 lines):
E> Quitting from lines 44-45 (C-download.Rmd) 
E> Error: processing vignette 'C-download.Rmd' failed with diagnostics:
E> Not Found (HTTP 404). Failed to API endpoint does not exist on this server. Please check your code for typos, or consult our API guide at http://guides.dataverse.org..
E> --- failed re-building ‘C-download.Rmd’
E> 
E> SUMMARY: processing the following file failed:
E>   ‘C-download.Rmd’
E> 
E> Error: Vignette re-building failed.
E> Execution halted

@wibeasley
Copy link
Contributor

@kuriwaki, I'm guessing that it was a temporarily outage with the win-builder server? It's running on my two local machines, and on on GitHub actions (r release: https://github.com/IQSS/dataverse-client-r/pull/104/checks?check_run_id=3129414679).

The failing build on Github Actions (R development) two days looks like it's related to the version of readr released the day before. I bet it's fine if you run it now. https://github.com/IQSS/dataverse-client-r/pull/104/checks?check_run_id=3129414663#step:8:3202

- remove reference to slides, which are not linked
- remove reference to previous speakers etc
- updat eto CRAN version
Copy link
Member Author

@kuriwaki kuriwaki left a comment

Choose a reason for hiding this comment

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

@wibeasley thank you for the comments. I brought back the talk notes with some updates, brought back devools::test with filter, and kept the makefile removed. I noticed the makefile referenced outdated vignettes.

(I started my own "review" by accident in trying to reply - and could not undo)

for-developers/developer-tasks.R Show resolved Hide resolved
- rhub failed because tibble is not available. Maybe now required after readr 2.0.0
@kuriwaki kuriwaki linked an issue Jul 24, 2021 that may be closed by this pull request
@kuriwaki
Copy link
Member Author

I will try to resubmit this as soon as rhub and Actions win_devel tests check off.

The readr installation failure is now gone but seems to be replaced by a tibble installation failure. (tibble is only used in a vignette when displaying a dowlonaded file).

I created a checklist #105 following usethis 2.0.1 (use_release_issue) but I may not complete all the new items it for now given that the priority should be to get it back on CRAN as it was previously.

@kuriwaki kuriwaki merged commit e550b1f into master Jul 26, 2021
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.

Return 0.3.8 to CRAN
2 participants