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

rOpenSci submission #27

Closed
13 of 19 tasks
JonasGeschke opened this issue Jul 30, 2018 · 55 comments
Closed
13 of 19 tasks

rOpenSci submission #27

JonasGeschke opened this issue Jul 30, 2018 · 55 comments
Assignees

Comments

@JonasGeschke
Copy link
Collaborator

JonasGeschke commented Jul 30, 2018

[preparation for submission, see details here]

Summary

  • What does this package do? (explain in 50 words or less):
    With rcites we provide an R client to access to the Speciesplus database. We provide functions to 1. access the Speciesplus taxon concept, and thereafter 2. get a species' legislation status, both from CITES and from the European Union, 3. get a species' country-wise distribution range, as listed in Speciesplus, and 4. get the references on which a Speciesplus listing is based.

  • Paste the full DESCRIPTION file inside a code block below:

Package: rcites
Type: Package
Title: R Interface to the Species+ Database
Version: 0.1.0
Authors@R: c(person("Jonas", "Geschke", role = c("aut"), email = "jonas.e.geschke@gmail.com", comment = c(ORCID = "0000-0002-5654-9313")),
  person("Kevin", "Cazelles", role = c("aut", "cre"), email = "kevin.cazelles@gmail.com", comment = c(ORCID = "0000-0001-6619-9874")),
  person("Ignasi", "Bartomeus", role = c("aut"), comment = c(ORCID = "0000-0001-7893-4389")),
  person("Jonathan", "Goldenberg", role = c("ctb")),
  person("Marie-Bé", "Leduc", role = c("ctb")),
  person("Yasmine", "Verzelen", role = c("ctb")))
Description: A programmatic interface to the Species+ <https://speciesplus.net/> database via the Species+/CITES Checklist API <https://api.speciesplus.net/>.
URL: https://ibartomeus.github.io/rcites/, https://github.com/ibartomeus/rcites
BugReports: https://github.com/ibartomeus/rcites/issues
License: MIT + file LICENSE
Depends:
  R (>= 3.1.0)
Imports:
  httr,
  data.table,
  methods
Encoding: UTF-8
RoxygenNote: 6.0.1
Suggests: knitr, rmarkdown, testthat, rworldmap
VignetteBuilder: knitr
  • URL for the package (the development repository, not a stylized html page):
    https://github.com/ibartomeus/rcites

  • Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):
    Database access, because the package gives access to the Species+/CITES Checklist API

  •   Who is the target audience and what are scientific applications of this package?  
    Researchers and national authorities

  • 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.

Requirements

Confirm each of the following by checking the box. This package:

  • does not violate the Terms of Service of any service it interacts with.
  • has a CRAN and OSI accepted license.
  • contains a README with instructions for installing the development version.
  • includes documentation with examples for all functions.
  • contains a vignette with examples of its essential functions and uses.
  • has a test suite.
  • has continuous integration, including reporting of test coverage, using services such as Travis CI, Coveralls and/or CodeCov.
  • I agree to abide by ROpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

Publication options

  • Do you intend for this package to go on CRAN?
  • Do you wish to automatically submit to the Journal of Open Source Software? If so:
    • The package has an obvious research application according to JOSS's definition.
    • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
    • The package is deposited in a long-term repository with the DOI:
    • (Do not submit your package separately to JOSS)
  • Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
    • The package is novel and will be of interest to the broad readership of the journal.
    • The manuscript describing the package is no longer than 3000 words.
    • You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see MEE's Policy on Publishing Code)
    • (Scope: Do consider MEE's Aims and Scope for your manuscript. We make no gaurantee that your manuscript willl be within MEE scope.)
    • (Although not required, we strongly recommend having a full manuscript prepared when you submit here.)
    • (Please do not submit your package separately to Methods in Ecology and Evolution)

Detail

  • Does R CMD check (or devtools::check()) succeed? Paste and describe any errors or warnings:

  • Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:

  • If this is a resubmission following rejection, please explain the change in circumstances:

  • If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
    https://github.com/karthik
    https://github.com/geanders

@JonasGeschke
Copy link
Collaborator Author

JonasGeschke commented Jul 30, 2018

this is the rOpenSci submission content. so we only need to copy and paste it for submission.

@KevCaz please re-check on the requirements (but as far as I got them they should be good) and do the details?

@KevCaz
Copy link
Member

KevCaz commented Jul 30, 2018

R CMD check is Ok (well we'll have the CRAN link anyway)

@JonasGeschke JonasGeschke changed the title rOpenSci submission: preparation rOpenSci submission Jul 30, 2018
@KevCaz
Copy link
Member

KevCaz commented Jul 30, 2018

We strongly suggest submitting your package for review before publishing on CRAN or submitting a software paper describing the package to a journal. Review feedback may result in major improvements and updates to your package, including renaming and breaking changes to functions.

Worst case scenario we'll published version 1.0.X afterwards.

@JonasGeschke
Copy link
Collaborator Author

yes, thats why I asked if you still consider submitting to rOpenSci. but anyways, so the first update probably will be a major one.

@KevCaz
Copy link
Member

KevCaz commented Jul 30, 2018

Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:

I'm checking with the goodpractice package as suggested.

@KevCaz
Copy link
Member

KevCaz commented Jul 30, 2018

So when using goodpractice::gp() I got the following outputs (I added my comments):

  ✖ write unit tests for all functions, and all package code
    in general. 17% of code lines are covered by test cases.

    R/sppplus_login.R:20:NA
    R/sppplus_login.R:24:NA
    R/sppplus_login.R:25:NA
    R/sppplus_simplify.R:22:NA
    R/sppplus_simplify.R:23:NA
    ... and 99 more lines

Actually we have 92% (there are tests skip because a token is required)

  ✖ avoid long code lines, it is bad for readability. Also,
    many people prefer editor windows that are about 80 characters

    wide. Try make your lines shorter than 80 characters

    inst/tmp/example.R:2:1
    inst/tmp/example.R:2:1
    inst/tmp/example.R:2:1
    inst/tmp/example.R:2:1
    inst/tmp/example.R:2:1
    ... and 35 more lines

I actually use formatR::tidy_source() with a cut_off of 80, so lines may be 85 max, I can change this and use a cutoff of 70.

  ✖ avoid sapply(), it is not type safe. It might return a
    vector, or a list, depending on the input data. Consider using
    vapply() instead.

    R/sppplus_simplify.R:23:20

This is the only worrisome I have so far. I'll need some time to think about this.

@JonasGeschke
Copy link
Collaborator Author

Hi @KevCaz did you find a way to deal with the latter issue or do you want/need @ibartomeus and me to have a look at it, too?

@KevCaz
Copy link
Member

KevCaz commented Aug 8, 2018

Actually I'm waiting for the response from CRAN before looking at this!

@JonasGeschke
Copy link
Collaborator Author

Ah okay alright, no worries! just wanted to check on it so we dont push it back for months again ;-)

@KevCaz
Copy link
Member

KevCaz commented Aug 11, 2018

I'll work on this tomorrow! Currently on the way back from ESA!

@KevCaz
Copy link
Member

KevCaz commented Aug 14, 2018

I think everything is in order for submission to Ropenscience!

@JonasGeschke
Copy link
Collaborator Author

Great, thanks for the info!

Submission is done: ropensci/software-review#244

@ibartomeus
Copy link
Collaborator

Great!! Thanks both!

@ibartomeus
Copy link
Collaborator

By the way, I am happy to prepare a blog post for MEE once we have a DOI and get some visibility.

@KevCaz
Copy link
Member

KevCaz commented Aug 17, 2018

That's great! :)

@JonasGeschke
Copy link
Collaborator Author

Yes thats good, thanks!
The review process started, until now its just the long code line issue that came up, but nothing to worry about now. Lets first wait for all the reviewers to be assigned and submit the reviews. You can follow up here: ropensci/software-review#244

@JonasGeschke
Copy link
Collaborator Author

JonasGeschke commented Aug 22, 2018

Review checklist to be implemented
I put the checklist here so we don't spam the reviewers too much. comment here if you start working on something, so we don't do double work.

Review 1, details see here: ropensci/software-review#244 (comment)

  • (01) make simplify more aggressive and provide option to receive unprocessed list (raw=FALSE)
  • (02) check special_cases for correct data structure
  • (03) consider returning data.frames rather than data.tables
  • (04) return simplified data.frames as objects
  • (05) make printing of long text fields more clean
  • (06) rename function prefixes for more straight forward understanding and workflow ROpenSci review: function prefixes #29
  • (07) consider a language parameter for cites_legislation, eu_legislation and distribution Add scope and language parameters #31
  • (08) consider a scope parameter for cites_legislation and eu_legislation Add scope and language parameters #31
  • (09) consider a bulk analysis for taxonconcept
  • (10) add more wording to the @return fields of documentation
  • (11) add introduction and basic information to README
  • (12) make the vignette more comprehensive, including code outputs + explanation of type/structure of information that is returned and workflows + explanation of simplify (see16) Make the vignette more comprehensive #32
  • (13) consider caching downloaded data locally for further processing (see 19)
  • (14) shorten lines in examples
  • (15) auto-pagination

Review 2, details see here: ropensci/software-review#244 (comment)

  • (16) work on vignette, find typos (see 12)
  • (17) explain input needed for set_token()
  • (18) explain the function prefixes in the readme
  • (19) improve processing time (see 13)
  • (20) consider making tibble output an option
  • (21) check spp_distribution() for class-data.frame-error

@JonasGeschke
Copy link
Collaborator Author

Also, my advice is to hold off on major changes prior to getting your second review. It's usually easier to address them simultaneously, and mcsiple (review 2) may not agree with me on everything!

This is a totally valid point from noamross (review 1). However, I guess some points are good to go or could have been expected, like some documentation issues and the function prefixes. Thus I still would continue with points 3 and 4 (dropping a dependency), 6, 10 and 11. the readme is pretty much done, I think we should guide people to the paper that is citable.

points 1 is an interesting one I think, but would need more work. same with 9, 12 and 13. 14 for now is a nice to have but no must have. points 7 and 8 would be nice, depending on how much work their implementation is.

I would say ...

@ibartomeus
Copy link
Collaborator

ok, what do you want me to focus on to be efficient? 3 and 4? Or this is something @KevCaz is already on it? Just say it and I'll do it this weekend. Maybe 12?

I don't think 13 is that important, as the queries run pretty fast, and the user can save them if needed.

@KevCaz
Copy link
Member

KevCaz commented Aug 24, 2018

I think we should address comments 1-5 together, I'll create a new branch for this.

@JonasGeschke
Copy link
Collaborator Author

I will check on 7 and 8. would be great if you have a look at 1-5.

@KevCaz
Copy link
Member

KevCaz commented Aug 24, 2018

Sounds good!

@KevCaz
Copy link
Member

KevCaz commented Aug 24, 2018

Can you add the auto-pagination in the to do list. I should be able to deal with that too!

@JonasGeschke
Copy link
Collaborator Author

The auto-pagination was part of the bulk analysis in 9. separated them, now its 15.

@JonasGeschke
Copy link
Collaborator Author

I am gone for holidays the next two weeks (from Friday on) but will do my best to be availably for some time. will add the comments from the 2nd review in the post above.

@JonasGeschke
Copy link
Collaborator Author

while adding the prefix information to the readme, just out of interest @KevCaz print() is not a helper function? and any special reason why r_cites_null_to_na() is not rcites_?

@KevCaz
Copy link
Member

KevCaz commented Sep 10, 2018

just out of interest @KevCaz print() is not a helper function?

I gathered several methods in print.R, so I prefer to keep them inside the same R file.

any special reason why r_cites_null_to_na() is not rcites_?

nope! better use rcites_, thanks @JonasGeschke, enjoy your vacation!

@ibartomeus
Copy link
Collaborator

I am available (e.g. Tuesday) if you are specific about what I should do.

@JonasGeschke
Copy link
Collaborator Author

JonasGeschke commented Sep 11, 2018

I gathered several methods in print.R, so I prefer to keep them inside the same R file.

@KevCaz thats totally fine, I just wandered about the function name. as print() is a base function, it would probably be better to also name it rcites_print().

@KevCaz
Copy link
Member

KevCaz commented Sep 17, 2018

Hi,

I added the new sets of tests! We are almost there!

I have drafted a answer to the reviewer in inst/assets/draft_answer.md. I think you should have a look and edit it as much as you want! We also need to think about what we should do to introduce the new features in the documentation of the package. To me, a vignette bulk_analysis is in order and I still need some time to think if we can easily find a way to use vectors of taxon_id.

So:

  1. let's improve the documentation
  2. let's see if we can vectorize (if too complicated let's wait for a new version and provide a workaround in a vignette)

@JonasGeschke
Copy link
Collaborator Author

Hi there, I am back from vacation. What do you need me to do / would you like me to help you with?
On Friday and the weekend I may have time.

@KevCaz
Copy link
Member

KevCaz commented Oct 3, 2018

Hi @JonasGeschke,

Hope you had a good break! What we need is improve the documentation. I may do further modifications in the code, but it should not affect the way we work with the package!

BTW, I forgot to answer your question about print, it is a method, so we should keep it as it!

@KevCaz
Copy link
Member

KevCaz commented Oct 9, 2018

@JonasGeschke @ibartomeus,

So I've implemented the vectorization. We now need to finish the vignettes and then we are good to go. @JonasGeschke would you mind completing the Elephant vignette? I'll finish the "bulk analysis" one whenever I find some time this week. Then it's basically done.

The last build may not be working as it sounds that the API is not responding right now. It happens, just have to wait for a while then it's back to normal.

@JonasGeschke
Copy link
Collaborator Author

JonasGeschke commented Oct 9, 2018

@KevCaz great, thank you!

@KevCaz
Copy link
Member

KevCaz commented Oct 15, 2018

Almost there, I have shortened some lines to make our code compliant to goodpractices.

@KevCaz
Copy link
Member

KevCaz commented Oct 15, 2018

Package build is currently failing on AppVeyor https://ci.appveyor.com/project/KevCaz/rcites/builds/19525698, looks like the culprit is stringi (we cannot fix this ourselves), I'm currently checking if the build passes on a windows device, then I'll send the response to the reviewer.

@KevCaz
Copy link
Member

KevCaz commented Oct 15, 2018

Looks good I'll proceed.

@KevCaz
Copy link
Member

KevCaz commented Oct 15, 2018

done! ropensci/software-review#244

@KevCaz
Copy link
Member

KevCaz commented Nov 12, 2018

@JonasGeschke and @ibartomeus looks like we are almost done, given the change we've made I think we should use v1.0.0 for the next release to CRAN.

@ibartomeus
Copy link
Collaborator

Amazing, thanks!

@JonasGeschke
Copy link
Collaborator Author

JonasGeschke commented Nov 13, 2018

@JonasGeschke and @ibartomeus looks like we are almost done

great, once again thanks for your immediate work on the comments coming in!

given the change we've made I think we should use v1.0.0 for the next release to CRAN.

yes, I totally think so, too. but lets wait with the next release until the whole ROpenSci process is done.

anyhow: download numbers looking very good!

@JonasGeschke
Copy link
Collaborator Author

JonasGeschke commented Nov 14, 2018

submission approved!

To-dos now (ropensci/software-review#244 (comment)):

  • (1) Please transfer the package to ropensci- I've invited you to a team within our ropensci organization. After you accept you should be able to move it. You'll be made admin once you do.
  • (2) add rOpenSci footer to README
  • (3) Change any needed links, such those for CI badges
  • (4) Travis CI should update to the new location automatically - you may have to update other CI systems manually
  • (5) add ropensci review badge to your readme
  • (6) We're starting to roll out software metadata files to all ropensci pkgs via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your pkg, after installing the pkg - should be easy as running codemetar::write_codemeta() in the root of your pkg

@JonasGeschke
Copy link
Collaborator Author

JonasGeschke commented Nov 14, 2018

1 has to be done by @ibartomeus I guess? as you are the only admin of the repository, right?
next to those to-dos, a next release and cran update is on. anything else @KevCaz ?

@JonasGeschke
Copy link
Collaborator Author

@KevCaz
Copy link
Member

KevCaz commented Nov 14, 2018

What about JOSS and a blog post?

@JonasGeschke
Copy link
Collaborator Author

What about JOSS and a blog post?

8.2.5 For joint JOSS submissions:

and then, yeah, we can write one blog post for ropensci and one for MEE.

@ibartomeus
Copy link
Collaborator

Yuhu! I started drafting a post here: https://docs.google.com/document/d/1ddyBaaSl8Q20tcjpTYMgAAUT7TP9LMRZgDNugC4ierM/edit?usp=sharing

Add whatever you want, its just a rough draft.

@JonasGeschke
Copy link
Collaborator Author

Thanks @ibartomeus; I know changed all the badge links.

When running write_codemeta() I get the error msg "Error: 'str_remove' is not an exported object from 'namespace:stringr'"
Same for you @KevCaz ?

@KevCaz would you also do the new release then? you are our technical expert :-)
For releasing with an doi we should use Zenodo I guess?

@KevCaz
Copy link
Member

KevCaz commented Nov 16, 2018

I'll take care of the next release on CRAN, I'll check the write_codemeta() as well and the DOI, I'm just wondering if we should get a DOI from Zenodo or from JOSS, I'll check that too.

@KevCaz
Copy link
Member

KevCaz commented Nov 16, 2018

Sounds like DOI first...

@JonasGeschke
Copy link
Collaborator Author

Thanks @KevCaz for the release and DOI!
Once you got the write_codemeta() done I will let sckott know about it – or you can. Then, once he says everything is good to go, I can do the JOSS submission.

@KevCaz
Copy link
Member

KevCaz commented Nov 17, 2018

@JonasGeschke I'm almost done with codemeta, I'll let sckott know about it, the next commit will close the issue, the new version (1.0.0) is on its way to CRAN.

@KevCaz
Copy link
Member

KevCaz commented Nov 17, 2018

360b95b adds codemeta.json. I'm closing this issue as the submission is now completed, if some discussions are required for the JOSS paper let's open a separate issue :)

@KevCaz KevCaz closed this as completed Nov 17, 2018
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

No branches or pull requests

3 participants