-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
osfr: R interface to OSF #279
Comments
thanks for your submission @aaronwolen @maelle has been assigned as editor |
Editor checks:
Editor commentsThanks for your submission @aaronwolen! Here are a few questions/comments before I can start looking for reviewers.
── Test failures ────────────────────────────────────────────────────────── testthat ────
> library(testthat)
> library(osfr)
Automatically registered OSF personal access token
>
> test_check("osfr")
── 1. Error: (unknown) (@test-osf_ls.R#5) ──────────────────────────────────────────────
Not found.
HTTP status code 404.
1: osf_retrieve_node("brfza") at testthat/test-osf_ls.R:5
2: raise_error(out)
3: http_error(x$status_code, x$errors[[1]]$detail)
4: stop(args, msg, call. = FALSE)
── 2. Error: (unknown) (@test-osf_tbl.R#37) ────────────────────────────────────────────
Not found.
HTTP status code 404.
1: osf_retrieve_node("brfza") at testthat/test-osf_tbl.R:37
2: raise_error(out)
3: http_error(x$status_code, x$errors[[1]]$detail)
4: stop(args, msg, call. = FALSE)
══ testthat results ════════════════════════════════════════════════════════════════════
OK: 90 SKIPPED: 1 FAILED: 2
1. Error: (unknown) (@test-osf_ls.R#5)
2. Error: (unknown) (@test-osf_tbl.R#37)
Error: testthat unit tests failed
Execution halted
1 error ✖ | 0 warnings ✔ | 0 notes ✔ I have not looked in detail, is it because this is a node that only exists in the account you use for testing? Couldn't the node be created before testing so that anyone can run the tests?
Reviewers: @HeidiSeibold @cboettig |
Hi, @maelle! Thanks for checking out the package and I apologize for the delayed response—I’ve been out traveling most of this week and am still playing catchup. I made the following changes based on your comments:
Regarding the test failures you encountered, I believe the problem was the tests were run on the main OSF server ( This really underscored that the testing infrastructure was a little fragile, so I did some restructuring and added checks to skip certain tests if they’re being run on the production server or if no PAT is defined (ropensci/osfr@81cfe71). It was a big enough change that I bumped the version to 0.2.2. Thanks again for looking it over and please let me know if you have any other questions or follow-up comments. |
Thanks @aaronwolen, great work! Sorry for missing the contributing guidelines! Two small suggestions:
I'll now look for reviewers! 😸 |
Great suggestions! I did not know about |
@aaronwolen Great definition of |
@HeidiSeibold @cboettig Thanks for accepting to review this package! 😸 Your reviews are due on 2019-02-20. As a reminder here's our guide for reviewers and the review template. |
@maelle, a new version of purrr was just released that, unfortunately, introduced a bug in osfr. It's fixed in the dev branch but I wanted to verify it's okay to merge the new version now that reviewers have been assigned? |
Congrats on fixing that bug so quickly! Yes I think it's fine, I don't think they started reviewing yet (feel free to disagree, @HeidiSeibold and @cboettig). In general we prefer a stable version but bug fixes are a good reason to change stuff. 😸 |
I will just add my review template below, so you know what I am up to 😸 Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 10 Review Comments
What happens for
> osf_auth(token = "<mytoken>")
Registered PAT from the provided token
> osf_retrieve_node("vk95n")
Error: User provided an invalid OAuth2 access token
HTTP status code 401. (Btw. the issue for me was that I was using the token from the test server instead of the actual osf, where the project lives)
https://osf.io/vk95n/ Any idea what is causing this?
@aaronwolen if you have time, I would appreciate you already answering my current questions. This way it will be easier for me to move forward. Thanks :) |
@HeidiSeibold live-updated review, that's open science for sure. 😉 |
Question: what is the difference between: The package README is instructing to use the former. Does that matter at all @maelle? |
@richfitz said " |
I am trying to build the package on my computer (Debian):
The pandoc issue is probably something I have to fix. How do you solve the authentication issue @aaronwolen? The reason why I wanted to build it myself was to have a nicely viewable Vignette, which I apparently don't get if I install from GitHub. > browseVignettes(package = "osfr")
No vignettes found by browseVignettes(package = "osfr") |
@HeidiSeibold I had the same authentication issues before @aaronwolen pointed me to https://github.com/CenterForOpenScience/osfr/blob/master/.github/CONTRIBUTING.md :-) |
Hi, @HeidiSeibold! I just noticed your edits and responded below:
The default behavior is to throw the following error
I've run into this several times myself. I'm considering adding a startup message that informs the user when
The I see now that this behavior is not documented! I'll improve the documentation for this argument in the dev branch. Thanks for pointing that out! Thanks for the questions! I'll keep checking back for more. |
@maelle I have trouble checking
and
because for many examples it does not make sense to run them without modification. I will just check them anyway because I can use and modify the examples so it works for me. |
@HeidiSeibold I'm not sure I understand your problem? |
Awesome! Note that there might be some delay on my side over the holidays (happy holidays everyone in this thread by the way 😉). |
@HeidiSeibold I've just realized you didn't get a chance to approve the package (the remaining box in your review) -- it's fine if you don't have time to. If you don't answer within a week I'll do a few more checks myself.
|
Thanks @maelle. I am really busy this week unfortunately. 🌻 |
Thanks, @maelle! I added in the missing bib file and I'm working on updating the docs. Thanks for the Rmd fragment tip! |
@HeidiSeibold Thanks, no problem, I'll re-read your review after @aaronwolen updates the docs. @aaronwolen I'm using Rmd fragments in codemetar dev branch and really liking them. Feel free to comment on their use at ropensci/dev_guide#159 |
Hi @maelle, I think I've addressed the remaining issues. Here's a quick summary of recent changes:
These are currently on the |
Approved, the changes look good! Thanks @aaronwolen for submitting and @HeidiSeibold @cboettig for your reviews! 😺 To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them Welcome aboard! We'd love to host a blog post about your package - either a short introduction to it with one example or a longer post with some narrative about its development or something you learned, and an example of its use. If you are interested, review the instructions, and tag We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here. |
The process took just less than one year, pfiew! 😉 |
🎉 🎉 🎉 That's great news, @maelle! Thank you so much for all of the help and keeping this process on track. And a sincere thanks to @cboettig and @HeidiSeibold for your great comments/suggestions. I think the package is much stronger for having gone through this process—it's certainly older 😄. I'll work on those remaining to-dos and double-check with the COS folks about migrating the repo to the ropensci org. |
Thanks, done! Once the repo is transferred, ping me so that I might give both of you admin rights again (note that my timezone is UTC+1). |
Repo successfully transferred, @maelle - https://github.com/ropensci/osfr |
Thanks, @mfraezz! |
@mfraezz thanks! I don't see you as collaborator of the repo but I gave admin rights to @aaronwolen. |
I'm an owner of the CenterForOpenScience organization, so it was implicit before. I'd recommend updating the repo description to include the new GH pages url, https://ropensci.github.io/osfr/ , but otherwise have no real need for access. |
@cboettig and @HeidiSeibold, any objections to being listed as reviewers in the package |
fine by me! |
I'd be honored :) |
@aaronwolen now that you've submitted the package to JOSS I think the only missing item from my list is your adding a codemeta.json to the repo (via |
Thanks, @maelle! Just added the missing |
Yes! ❤️ it! I discovered it last week week and try really hard not to check it daily. |
Submitting Author: Aaron Wolen (@aaronwolen)
Repository: https://github.com/CenterForOpenScience/osfr
Version submitted: 0.2.1
Editor: TBD
Reviewer 1: TBD
Reviewer 2: TBD
Archive: TBD
Version accepted: TBD
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how the and why the package falls under these categories (briefly, 1-2 sentences):
osfr provides a suite of functions for interacting with OSF (https://osf.io), a project management repository that supports researchers across their entire project lifecycle. Using osfr and OSF, researchers can create and manage projects, upload and share materials, or explore publicly accessible projects and download the associated files.
Who is the target audience and what are scientific applications of this package?
Any R users who rely on OSF for managing their research or working with collaborators.
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
No.
If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Technical checks
Confirm each of the following by checking the box. This package:
Publication options
JOSS Options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: