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

added more verbose error message to httr::stop_for_status #30

Merged
merged 10 commits into from
Feb 26, 2021
Merged

added more verbose error message to httr::stop_for_status #30

merged 10 commits into from
Feb 26, 2021

Conversation

EdJeeOnGitHub
Copy link
Contributor

Returns Dataverse API error message to make debugging easier.

  • R CMD Check passes locally with the change.

@EdJeeOnGitHub EdJeeOnGitHub mentioned this pull request Dec 1, 2019
Closed
@EdJeeOnGitHub
Copy link
Contributor Author

I opened an issue here with a bit more detail.

@pdurbin pdurbin added this to Don't forget in pdurbin Dec 2, 2019
@wibeasley
Copy link
Contributor

@EdJeeOnGitHub, I haven't forgotten about this PR. I see that some of the master branch has changed since your modification, and I'll do my best to make sure you get credit.

Will you please

  1. add yourself to the DESCRIPTION file as a "ctb". Add your ORCID id if you have one.
  2. update NEWS.md to reflect this improvement?

I'm using R Packages for the definition/distinction between the "aut" and "ctb".

@EdJeeOnGitHub
Copy link
Contributor Author

I think I managed to fix the conflicts too?

@pdurbin pdurbin removed this from Don't forget in pdurbin Jul 10, 2020
@kuriwaki kuriwaki linked an issue Dec 28, 2020 that may be closed by this pull request
More verbose httr error message for get_file() #31
Closed
@kuriwaki
Copy link
Member

I like this simple addition and support working to merging this PR. I added a testthat case. See also my comment #31 (comment) on the associated Issue that seeks to find one more informative test that is compatible with current master version.

@kuriwaki
Copy link
Member

Also, shouldn't we implement the same change in the dozens of other places where httr::stop_for_status is called -- not just in get_file()?

@kuriwaki
Implement @EdJeeOnGitHub's verbose messaging on all instances
547ff90
@kuriwaki
Pull upstream
7180656 
Merge remote-tracking branch 'upstream/master' into EdJeeOnGitHub-master

# Conflicts:
#	DESCRIPTION
#	NEWS.md
#	R/get_dataset.R
#	R/get_file.R
@kuriwaki
Add message to the new calls for stop_for_status
cc8dabf
@kuriwaki
Reconcile
7d0ac8c
@wibeasley wibeasley merged commit f6715c6 into IQSS:master Feb 26, 2021
@kuriwaki kuriwaki mentioned this pull request Mar 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

More verbose httr error message for get_file()
3 participants
@EdJeeOnGitHub @wibeasley @kuriwaki