Open eyrexh opened 1 year ago
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
).Estimated hours spent reviewing: 1hr
Great package!
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
).Estimated hours spent reviewing: 1
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
).Estimated hours spent reviewing: ~ 1.5 hours
(Review based on the latest commit on main
as of writing, commit https://github.com/UBC-MDS/nameformeR/commit/50948086ff64a5e6c0a70364d0dd0f1793bfa701)
First, congratulations on the releasing of the package. It is a fun, simple (yet cool!) idea to use this dataset to suggest baby names.
What I like most is the code is clean and neat. I can see that other reviewers have already given excellent comments over the functionality aspect over the package. I would therefore would like to focus solely on the code-side of things instead.
(Some comments are similar to what I have made in the Python package, which can be viewed here.)
1. Consider DRYing your code, instead of Writing Every Time.
An example would be the snippet of loading the CSV dataset:
url <- "https://raw.githubusercontent.com/rfordatascience/tidytuesday/master/data/2022/2022-03-22/babynames.csv"
data <- readr::read_csv(url,col_types = readr::cols())
This piece of code appears for 4 times. You may consider refactoring by, e.g., wrapping it as an utility function:
load_raw_data <- function () {
url = "https://raw.githubusercontent.com/rfordatascience/tidytuesday/master/data/2022/2022-03-22/babynames.csv"
readr::read_csv(url, col_types = readr::cols())
}
While the line count does not seem to decrease much, the real benefit is that when we want to, say, change the upstream dataset which may have a different structure, it would be easier.
2. find_unisex_name
has a conflicting Roxygen definition.
The said function is defined as find_unisex_name <- function(bar, limit = 10)
, but provides the parameters as:
#' @param limit A float controls the minimum proportion of the neutral names in a single year in the dataframe.
#' @param bar The length of the output with a default value of 10.
It would be better to reorder the listed parameters accordingly, with documentation review.
3. find_old_name
should have better guard.
I realized this when I tried out the sex
parameter in it, but realized that this does not work with lower cases (e.g., 'f'
or 'm'
), unlike find_name
. The offending piece of code is:
df <- data |> dplyr::filter(tp == {{tp}})
if (sex == "uni"){
f <- df |> dplyr::filter(sex=="F") |> dplyr::pull(name)
m <- df |> dplyr::filter(sex=="M") |> dplyr::pull(name)
uni_df <- intersect(f,m)
if (length(uni_df) < limit){
uni_df
}else{
sample(uni_df,size=limit,replace = FALSE)
}
}else{
r <- df |> dplyr::filter(sex=={{sex}}) |> dplyr::pull(name)
sample(r,size=limit,replace = FALSE)
}
Similar to the Python counterpart, it can be rewritten to simplify and with added guards. May refer to the Python issue for more details.
4. Consider enhancing your test cases, especially the conditional guards.
Looking for 100% line coverage for all projects is impractical, but it is always beneficial to cover edge cases. For example, currently the line coverage report shows that we are missing 10 lines, which are related to error handling cases (e.g., throwing exception when the type is not expected, out of range). It would be beneficial that this kind of behaviors to be documented in form of a working test case.
5. find_name
and find_old_name
have different sex
definition, which can be confusing.
It might be best if the parameter can be unified. For example:
"M"
or "m"
for Male"F"
or "f"
for FemaleNULL
for UnisexOverall, I enjoy reading the code of your project, and despite hiccups, your documentation is good. Great work!
(Reviewed using rOpenSci review template
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
).Estimated hours spent reviewing:
name: nameformeR about: A helper python package that can be used to generate names. This could be used to come up with baby names, character names, pseudonyms, etc.
Submitting Author Name: Daniel Cairns, Eyre Hong, Bruce Wu, Zilong Yi Submitting Author Github Handle: @DanielCairns @eyrexh @BruceUBC @ZilongYi
Repository: https://github.com/UBC-MDS/nameformeR Version submitted: v1.1.0 Submission type: Standard Editor: Daniel Cairns (@DanielCairns), Eyre Hong (@eyrexh), Bruce Wu (@BruceUBC), Zilong Yi (@ZilongYi) Reviewers: @ranjitprakash1986, @Althrun-sun, @netsgnut, @kellywujy
Archive: TBD Version accepted: TBD Language: en
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 inquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Explain reasons for any
pkgcheck
items which your package is unable to pass.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