-
-
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
rdflib #169
Comments
Editor checks:
Editor commentsPasses
Here is the result of
97% coverage is excellent. The advice not to import entire packages is up to the discretion of the reviewers. Note one misspelling ( @karthik I am now approaching reviewers. Reviewers: |
Two reviewers have agreed to review this package. Reviewers, thanks for being willing, and I'll ask you to have your reviews in within three weeks. Here is the reviewer's guide. Feel free to let me know if you have any questions. Reviewer: Anna Krystalli, @annakrystalli @cboettig Could you please add the rOpenSci under review badge to the README for this package? Here is the snippet.
|
Package Review
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 2 Review CommentsThis looks like an excellent package for inclusion in the ropensci ecosystem. I have personal experience with the problem this package is trying to solve, namely working with RDF in R from the analysts perspective. The RDF package this package wraps, The package is laid out in a non-surprising manner, most functions are short and well-scoped, and, overall, the code is very readable. The accompanying test suite is reasonable and provides 100% coverage, and the single vignette is well-written and useful. I did find the documentation could use some polish in some places (see comments below). I suspect a pass or two by the author would make some good improvements without much work. I have left two checkboxes unchecked due to the following issues:
but I otherwise found everything else to be in order. Higher level
Lower levelThese were written out as I went through the checklist:
|
Thanks for getting your review in, @amoeba. |
Hello all and apologies for the delay! Here is my review: 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: 7 Review CommentsThis package is a great and lightweight addition to working with Overall I feel package functionality is complete and self-contained (apart from one error identified below). My main feedback is regarding documentation, specifically how it could be improved to help novice users to grasp the value of semantic data and better understand how the package works. installationThe only install comment I'll add is that when I first ran pkg_dir <- "/Users/Anna/Documents/workflows/rOpenSci/reviews/rdflib-review/../rdflib"
devtools::install(pkg_dir, dependencies = T, build_vignettes = T)
#> Installing rdflib
#> '/Library/Frameworks/R.framework/Resources/bin/R' --no-site-file \
#> --no-environ --no-save --no-restore --quiet CMD build \
#> '/Users/Anna/Documents/workflows/rOpenSci/reviews/rdflib' \
#> --no-resave-data --no-manual
#>
#> Error: Command failed (1) with the console output:
Installing without building the vignette results in successful installation of pkg_dir <- "/Users/Anna/Documents/workflows/rOpenSci/reviews/rdflib-review/../rdflib"
devtools::install(pkg_dir, dependencies = T)
#> Installing rdflib
#> '/Library/Frameworks/R.framework/Resources/bin/R' --no-site-file \
#> --no-environ --no-save --no-restore --quiet CMD INSTALL \
#> '/Users/Anna/Documents/workflows/rOpenSci/reviews/rdflib' \
#> --library='/Users/Anna/Library/R/3.4/library' --install-tests
#>
#> Installing jqr
#> '/Library/Frameworks/R.framework/Resources/bin/R' --no-site-file \
#> --no-environ --no-save --no-restore --quiet CMD INSTALL \
#> '/private/var/folders/8p/87cqdx2s34vfvcgh04l6z72w0000gn/T/RtmpbYeNu9/devtools6d3b4a7582a1/jqr' \
#> --library='/Users/Anna/Library/R/3.4/library' --install-tests
#> if tests and checksAll OK documentationMy main suggestion is to try to define some terms and improve the concept map for the tools by adding some detail and broader context to the documentation. The following suggestions could also be addressed with links to further details if you think they are too superfluous for explicit documentation with the package.
Spelling a few things out in plain english and explicitly could really help folks follow what's going better and understand what file types are inputs or outputs of different functions. how do I find info on URIs?Some signposting/guidance on how I can find information on the semantics dictating what information I can extract from an examples in generalFor clarity to the reader who may not have looked at function documentation yet, I recommend using the full argument names when supplying arguments to functions (if not always atleast the first time an argument is introduced) in vignettes. SPARQL queries to JSON data sectionAt the end of the intro to the section, you write:
Am I right in thinking though that you are co-author on all papers in the rdf but the query is in fact filtering the names of your co-authors? (through Turning RDF-XML into more friendly JSONIt would be nice if possible to see sample of print outs of the conversion of the different files or at least of the effect of compaction.
|
Thanks for getting your review in, @annakrystalli. Now that both reviews are in, could you respond to the reviews and make changes as necessary, @cboettig? If possible, please do so within 2 weeks, which would be February 13. |
@lmullen @annakrystalli @amoeba Thanks for your reviews! I've just about finished addressing the issues raised at this point, which I've summarized in: A summary of the changes can be found in NEWS.md, which ended up being reasonably involved because the reviews got me thinking about a bunch of stuff, which was awesome. However, most substantive is perhaps the development of a new vignette, which I've liberally titled A tidyverse lover’s intro to RDF. This tries to address the big-picture issues Anna in particular highlights regarding documenting and motivating the broader context of RDF. This is still a bit more of a draft than a polished document, but given that my two weeks are up I think it might be a good time to get feedback on this (and the other changes) from the reviewers. In particular, I would love to hear what the reviewers think of this as a broader introduction. If the reviewers are interested and think it would be worthwhile, I believe it might be nice to overhaul this new vignette into a more general purpose intro to RDF for R users (both the relevant packages and concepts) that might be suitable for a submission to something like the R Journal. I'd love entice Anna and Bryce to be co-authors if they are interested... |
@cboettig Thanks for getting your review in on time, and for the thoroughness of the changes and how you reported them. I'm looking forward to reading the new vignette. @amoeba and @annakrystalli: Could you please go over the changes to the package and report back within one week? That would be by Thursday, February 22. I'll do the same. |
Hey @lmullen and @cboettig: I've reviewed the responses and changes @cboettig has made in response to my review and I every issue I raised has been addressed. I have no remaining issues and recommend the submission be accepted as modified. @lmullen would you like us to review the new vignette before acceptance? That's fine with me and I can certainly do that within the week. @cboettig I'm super excited with the direction you're taking. I'd certainly like to continue working on this package and a paper. In particular, this clicks for me:
I'd never before seen the equivalence between tidy data principles and RDF. I'll follow up with you elsewhere. |
Hi all 😃 I am really happy with the changes made and the direction of the vignette! triplestores are indeed the ultimate tidy data! A great way to sell it. It's already a great resource and am also happy to contribute to both the vignette and a paper on it. I'll feedback to some of the discussions raised in @cboettig response in So ✅ and big 👍 from me also. |
@ameoba: Yes, if you could please offer whatever suggestions you think are necessary on the new vignette that would be great, but it seems like we are very close to being done. |
Okay, will do. I'll get those comments in this week. |
Hi @cboettig. Thanks for the thoroughness of your response to the reviews. At this point I don't see any reason to delay accepting the package into rOpenSci. Of course it looks like you are still figuring out the final form of a few things, especially in the new vignette, so it will be your call when to submit to CRAN. @karthik I don't have access to rOpenSci admin accounts, so could you please begin the process of moving this package into the rOpenSci organization? After that happens, @cboettig, could you please do the following?
Once the repository is moved I will close this issue. |
Thank you @lmullen! Since Carl has ownership rights on the org (unlike most authors) he should be able to move this himself. |
Thanks @lmullen and @karthik! I've added the ropensci footer and migrated the repo to CI seems to be working (if I recall correctly I shouldn't migrate the appveyor since it only links to individual accounts?) I assume there's nothing I need to do to update the onboarding badge, that happens automatically via tags on this issue, right? I'll leave it to you editors to close out this thread when ready. |
@cboettig Great! Looking forward to using this myself the next time I need to deal with RDF. Special thanks to @amoeba and @annakrystalli for doing the review. |
Summary
rdflib
is simply a wrapper around two existing ropensci packages:redland
andjsonld
, which should be a user-friendly complement to the low-level interface already provided byredland
for working with RDF (semantic/linked data).https://github.com/cboettig/rdflib
data extraction, because this package parses scientific data file formats. (specifically, formats already parsed by existing rOpenSci packages). This package also enables graph queries using the SPARQL language, somewhat analogous to the rOpenSci
jqr
package, but for JSON-LD and other linked data formats.Anyone working with semantic data, including the wide array of scientific ontologies and knowledge-bases. These include reproducibility-focused ontologies like PROV, and a large number of biological ontologies ranging from genes to traits to environmental features.
yours differ or meet our criteria for best-in-category?
As described above, this package overlaps significantly with the
redland
package, but should be easier to use.Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.Detail
Does
R CMD check
(ordevtools::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:
The text was updated successfully, but these errors were encountered: