Open camharris22 opened 3 years ago
Assigning @Saule-Atymtayeva & @nichowu as reviewers.
Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).For packages co-submitting to JOSS
- [ ] The package has an obvious research application according to JOSS's definition
The package contains a
paper.md
matching JOSS's requirements with:
- [ ] A short summary describing the high-level functionality of the software
- [ ] Authors: A list of authors with their affiliations
- [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
- [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).
Estimated hours spent reviewing: 2
Hi Guanshu, Cameron, Jayme and Zhiyong,
Congrats on completing the package. I like the idea of converting numbers into human or machine readable format. The functions in this package are easy to use and the documentation is clear for the most part. I was able to successfully run to_human
, to_numeric
and to_df
. The output makes sense and I appreciate the toy data provided.
There are just a couple of suggestions on small details I wanted to bring to your attention:
to_human(78950000, prec = 0, family = "numeric") Error in check_family(family) : Family not in suffixes
Your to_color
function strictly requires an integer to be passed in, so I received an error when I tried:
to_color(1234567, c("red", "green", "yellow", "blue")) Error in to_color(1234567, c("red", "green", "yellow", "blue")) : Can only support integer number
This could be confusing to some people because "1234567" looks like an integer in our human eyes. To make the function more accessible to most, I would convert the number into an integer within the function, instead of throwing an error to the user.
Another thing with to_color
function is that I couldn't quite understand the output of the function. For example: when I run this:
to_color(1234567L, c("red", "green", "yellow", "blue"))
The output is:
"\033[32m1\033[0m\033[33m2\033[0m\033[34m3\033[0m\033[31m4\033[0m\033[32m5\033[0m\033[33m6\033[0m\033[34m7\033[0m"
I know that these are standard terminal color codes, but I'm not sure how one could utilize the output of this function. In your documentation, it says "The function will return a string that can be used in print() function to visual numbers with colors in the terminal". Can you clarify on that?
Your test functions seem to be thorough and the code in general are formatted properly. Good job on that!
Thanks,
Nick
Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
The package includes all the following forms of documentation:
Please, read the comments below.
Please, read the comments below.
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).For packages co-submitting to JOSS
- [ ] The package has an obvious research application according to JOSS's definition
The package contains a
paper.md
matching JOSS's requirements with:
- [ ] A short summary describing the high-level functionality of the software
- [ ] Authors: A list of authors with their affiliations
- [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
- [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).
Please, read the comments below.
Estimated hours spent reviewing: 3.
Hi NiceNumbERRR team,
Congratulations! Your work is really interesting, and I like how the numbers look after applying your functions.
I have some suggestions that might be useful in improving your project:
README
and Vignette homepage
documents, I would remove the part that says that the package can be installed from CRAN. I double-checked the availability of your package on CRAN in case there were any problems with the installation on my side, but I could not find the package.README
and Vignette homepage
documents for the to_human
function (to_human(78950000, prec = 0, family = "numeric")
) threw the error saying that Family not in suffixes
. I changed family = "numeric"
into family = "number"
to match the function input arguments, and it worked. So, you need to update this example in both documents. Also, I should mention that in the example you used install.packages("NiceNumbERRR")
instead of devtools::install_github("UBC-MDS/NiceNumbERRR")
.to_color
function I would paste a screenshot so that users could see your expected "colored" results of applying the to_color
function.Non-standard files/directories found at top level: ‘NiceNumbERRR_reviewform’ ‘docs’
note is here. The same solution will work for the NiceNumbERRR_reviewform
file as well (I tried usethis::use_pkgdown(config_file = "_pkgdown.yml", destdir = "NiceNumbERRR_reviewform")
, and it worked smoothly).> checking for unstated dependencies in vignettes ... NOTE '::' or ':::' import not declared from: ‘tibble’
note you can add tibble
in the Imports
section of DESCRIPTION
.return(n * base**(which(custom_suff == unit)))
), so I would create one more test to cover 100% of the relevant code lines.All the best, Saule
It seems like Nick and I have a couple of overlapping suggestions 🙂
@nichowu some clarification about to_color() function:
1234567
actually is a float/double type which I also feel surprised, 1234567L
is integer type, and our function only support integer input to make it simple@Saule-Atymtayeva thank you for the suggestion
name: NiceNumbERRR about: This R package provides basic functions that make numbers display nicely.
Submitting Author: Cameron Harris (@camharris22)
Other Authors:
Repository: NiceNumbERRR Version submitted: v0.3.0 Editor: TBD Reviewers:
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 and why the package falls under these categories (briefly, 1-2 sentences):
Who is the target audience and what are scientific applications of this package?
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 applicable) Does your package comply with our guidance around Ethics, Data Privacy and Human Subjects Research?
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
[ ] Do you intend for this package to go on CRAN?
[ ] Do you intend for this package to go on Bioconductor?
[ ] Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
MEE Options
- [ ] 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](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will 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*)Code of conduct