-
-
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
Submission: refnet #256
Comments
Thanks very much for your submission @aurielfournier - we're discussing now and will get back to you soon |
Thanks for your submission @aurielfournier! I see the package doesn't have any test and doesn't have continuous integration yet. I suggest we put the submission on hold while you sort that, unless you can and want to add this within a week or so? There is some guidance in this guide, and I am happy to answer any question here or via Slack! I was also looking at the dependencies, there are many of them in DESCRIPTION and
|
@aurielfournier for info I've just added the holding label, please update this thread once you have had time to work on the package, and ask me any question. |
Thanks @maelle I and my co-authors are working on it, but its taken a bit longer then we expected. Appreciate your patience! |
No problem, and happy to help if I/we can! |
@maelle package now has tests and continuous integration. I removed stringi from the DESCRIPTION file. I have fixed the issues from the NAMESPACE file. Huge thanks to my coauthor @birderboone for doing the heavy lifting to get this over the finish line! I think we are ready for review now. If you have any other things that need to be addressed let me know. Thanks! |
👋 @aurielfournier @birderboone! Awesome, thanks to both of you! A few comments before I do the last editor checks:
|
Hi @maelle moved the badge Addressed the Travis warnings/notes I closed the two open issues. thanks for pointing out the milestones, I had forgotten about that. I added the CoC and contribution guides. Thank you so much for all the links and tips on how to address these issues, it is greatly appreciated. Build is now passing!! |
Yay, green badge! Can you also add a coverage badge? Run |
Done! Sorry I missed that. |
Thank you! A few more things before I search for reviewers (then they and you have less work 😉).
Editor comments
✖ write short and simple functions. These
functions have high cyclomatic complexity:authors_clean
(68). Maybe you can split it in several helper functions? ✖ omit "Date" in DESCRIPTION. It is not required
and it gets invalid quite often. A build date will be
added to the package when you perform `R CMD build` on it. ✖ add a "URL" field to DESCRIPTION. It helps users
find information about your package online. If your
package does not have a homepage, add an URL to GitHub, or
the CRAN package package page.
✖ add a "BugReports" field to DESCRIPTION, and
point it to a bug tracker. Many online code hosting
services provide bug trackers for free,
https://github.com, https://gitlab.com, etc. Run ✖ 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
R\authors_clean.R:24:1
R\authors_clean.R:26:1
R\authors_clean.R:29:1
R\authors_clean.R:35:1
R\authors_clean.R:36:1
... and 162 more lines It's the complicated function, one more reason to try and simplify it? ✖ omit trailing semicolons from code lines. They
are not needed and most R coding standards forbid them
R\authors_refine.R:20:198 ✖ 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\authors_clean.R:97:17
R\authors_clean.R:132:19
R\authors_clean.R:152:19
R\authors_clean.R:186:17
R\authors_clean.R:223:22
... and 14 more lines
✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...),
1:NROW(...) and 1:NCOL(...) expressions. They are error
prone and result 1:0 if the expression on the right hand
side is zero. Use seq_len() or seq_along() instead.
R\authors_clean.R:56:15
R\authors_clean.R:71:79
R\authors_clean.R:188:15
R\authors_clean.R:209:21
R\authors_clean.R:422:12
... and 9 more lines
✖ avoid 'T' and 'F', as they are just variables
which are set to the logicals 'TRUE' and 'FALSE' by
default, but are not reserved words and hence can be
overwritten by the user. Hence, one should always use
'TRUE' and 'FALSE' for the logicals.
R/authors_clean.R:NA:NA
R/authors_clean.R:NA:NA
R/authors_clean.R:NA:NA
R/authors_clean.R:NA:NA
R/authors_clean.R:NA:NA
... and 38 more lines
It'll turn green when your package is approved.
|
Hi @maelle We are going to pause, and redo Thanks! |
Ok, thank you! |
Alright! After some fighting with Travis the past 24 hours, we are good to go. @birderboone split up authors_clean into three smaller internal functions, that should make review of the code easier. We've also addressed the other comments from @maelle If I missed something, let me know. Thanks! |
Thanks @aurielfournier and @birderboone! A few more things from It is good practice to
✖ add a "BugReports"
field to DESCRIPTION, and point
it to a bug tracker. Many
online code hosting services
provide bug trackers for free,
https://github.com,
https://gitlab.com, etc. Simply run ✖ use '<-' for
assignment instead of '='. '<-'
is the standard, and R users
and developers are used it and
it is easier to read your code
for them if you use '<-'.
tests\testthat\test_authors_match.R:4:5
The ✖ 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
R\authors_address.R:12:1
R\authors_address.R:14:1
R\authors_address.R:17:1
R\authors_address.R:20:1
R\authors_address.R:41:1
... and 174 more lines ✖ 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\plot_net_address.R:32:26
R\plot_net_address.R:33:26
tests\testthat\test_references_read.R:10:17
✖ avoid 1:length(...),
1:nrow(...), 1:ncol(...),
1:NROW(...) and 1:NCOL(...)
expressions. They are error
prone and result 1:0 if the
expression on the right hand
side is zero. Use seq_len() or
seq_along() instead.
R\authors_georef.R:55:25
R\authors_georef.R:71:15
R\authors_georef.R:113:17
R\plot_net_address.R:34:35
R\plot_net_address.R:123:22
... and 1 more lines
✖ fix this R CMD check
NOTE: Namespaces in Imports
field not imported from:
'Rdpack' 'maps' 'stringr' All
declared Imports should be
used.
✖ avoid 'T' and 'F', as
they are just variables which
are set to the logicals 'TRUE'
and 'FALSE' by default, but are
not reserved words and hence
can be overwritten by the user.
Hence, one should always use
'TRUE' and 'FALSE' for the
logicals.
R/authors_address.R:NA:NA
R/authors_address.R:NA:NA
R/authors_address.R:NA:NA
R/authors_georef.R:NA:NA
R/authors_georef.R:NA:NA
... and 15 more lines And from me: could you please add a coverage badge? Thanks in advance and thanks for all your work until now! 😸 |
Hi @maelle Thanks as always for your kind patience. It is greatly appreciated. I've addressed all of the above, and I finally downloaded The one issue that I don't totally understand, but isn't throwing an issues in
I thought that meant that I needed to remove But otherwise I think we're ok. :D |
Hi @maelle, I think we got all of these taken care of.
|
Hi @embruna, thanks! Np reg the remaining long lines. Reg DESCRIPTION could you have a look at dependencies ‘Rdpack’ ‘gdtools’ ‘mapproj’ ‘vdiffr’ as noted in my previous comment? Dependencies only used in tests should be under Suggests. by the way the line for Imports is very long, I'd suggest running Reg Travis what I meant is that I recommend undoing this commit ropensci/refsplitr@92e9fe5 since your package now is in very good shape, it'd be good for Travis to flag new WARNINGs as they appear. After that we'll be done. Thanks for your patience and thanks again for your work! |
OK, drum roll...
We're the ones who are grateful, both for your patience and the attention to detail. The review of this package has been incredibly helpful and detailed....I wish my papers were reviewed this thoroughly! |
Thank you! 😁 And thanks again to both reviewers! I'm sorry but I now realize these packages are maybe not even used at all in your package, or am I missing something? I've searched for their name in the package code (e.g. https://github.com/embruna/refsplitr/search?q=mapproj&unscoped_q=mapproj&type=Code)
If the packages are really not used, could you please remove them from Suggests? |
You don't need covr either since it's run on Travis. I've now found a way to find unneeded dependencies using the attachment package from inside the refsplitr folder. desc_deps <- attachment::att_from_description()
scripts_deps <- attachment::att_from_rscripts(".")
namespace_deps <- attachment::att_from_namespace()
vignette_deps <- attachment::att_from_rmds()
desc_deps[!desc_deps %in% c(scripts_deps, namespace_deps, vignette_deps)]
|
Note that you could still have false positives since you could use Suggests packages for other things. |
Happy New Year! @embruna Once you've responded to my comment about Suggests unneeded dependencies the package will be approved. 🙂 |
covr, mapproj, Rdpack, vdiffr as per ropensci/software-review#256 (comment)
Hi all, happy New Year. Deleted the 4 packages from the Suggests....thanks @maelle! |
Ah indeed sorry mapproj seems needed https://travis-ci.org/embruna/refsplitr/jobs/633881379#L6325 |
|
Yes, I just reinserted to the Suggests. Fingers crossed! |
Cool! I'll come back to this thread tomorrow, it's getting late here. |
Approved! Thanks @embruna @aurielfournier @birderboone for submitting and @njahn82 @bmkramer 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. |
Hi All, a quick update: The following have been COMPLETED
I'm finishing the following as soon as I am done with class:
At that point I will create the Zeonodo 1.0 version and mint the DOI, and submit to JOSS. Should be done shortly. |
Hi @noamross and @maelle, a couple quick questions:
|
|
OK, @noamross and @maelle: refsplitr should be ready for Zendo v1.0.0 The only unexpected thing that came up was that our title (or rather, the text after the colon in
I was a actually a bit concerned because Clarivate's webpage is clear:
However, a quick search on CRAN reveals several other packages wth "Web of Science" in the title, so I just left it in. If you prefer it be removed let me know. Otherwise, I will freeze v1.0.0 with Zenodo at the end of the day. |
I think the package isn't a company product? But I'm not a lawyer. In any case I think you'll need to write it |
Submitted to JOSS, and Zenodo archived as v0.9.0 with doi 10.5281/zenodo.3608285. Once the JOSS review is complete and I can include the details in the paper.rm file. At that point I'll freeze v1.0. A minor thing, and maybe unnecessary. But I wanted 1.0 to be a complete package with no details to be filled in. |
Ok! I don't see the submission at https://joss.theoj.org/papers yet. |
Yes, still listed as "pending"...I'll check on it periodically throughout the day. |
I've just checked again. Please ping me if you see it appear. 😉 |
Paper accepted at JOSS 🎉 so closing this issue. |
Summary
What does this package do? (explain in 50 words or less):
refnet
is a package to read, organize, geocode, analyze, and visualize Clarivate Web of Knowledge/Web of Science, format reference data files for scientometric, social network, and Science of Science analyses.Paste the full DESCRIPTION file inside a code block below:
URL for the package (the development repository, not a stylized html page):
https://github.com/embruna/refnet
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.):
Data extraction and munging, since it takes data from one format, and transforms it into something that is useful, and also matches up records among authors.
[Note, the link for the package fit, does not lead to that page anymore, and I couldn't find anything about package fit in the linked policies]
Scientists interested in studying the networks of a particular author, subject area or journal.
Are there other R packages that accomplish the same thing? If so, how does
yours differ or meet our criteria for best-in-category?
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.
#247
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
[yes] Does
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings:[yes] 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:
Heather Piwowar @hpiwowar
The text was updated successfully, but these errors were encountered: