rhud: A R interface for the US Department of Housing and Urban Development APIs #524
Comments
|
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type |
|
🚀 Editor check started 👋 |
Checks for hudr (v0.1.0.9000)git hash: 660ad0e2
Important: All failing checks above must be addressed prior to proceeding Package License: GPL (>= 2) 1. Statistical PropertiesThis package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing. Details of statistical properties (click to open)
The package has:
Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages
All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by the The final measure (
1a. Network visualisationClick to see the interactive network visualisation of calls between objects in package 2.
|
| name | conclusion | sha | date |
|---|---|---|---|
| pages build and deployment | success | 660ad0 | 2022-04-01 |
| R-CMD-check | success | 660ad0 | 2022-04-01 |
| test-coverage | success | 06b739 | 2022-03-28 |
3b. goodpractice results
R CMD check with rcmdcheck
R CMD check generated the following error:
- Error in proc$get_built_file() : Build process failed
R CMD check generated the following check_fail:
- no_import_package_as_a_whole
Test coverage with covr
ERROR: Test Coverage Failed
Cyclocomplexity with cyclocomp
Error : Build failed, unknown error, standard output:
- checking for file ‘hudr/DESCRIPTION’ ... OK
- preparing ‘hudr’:
- checking DESCRIPTION meta-information ... OK
- installing the package to build vignettes
- creating vignettes ... ERROR
--- re-building ‘authors.Rmd’ using rmarkdown
--- finished re-building ‘authors.Rmd’
--- re-building ‘Community-Development-Block-Grant.Rmd’ using rmarkdown
--- finished re-building ‘Community-Development-Block-Grant.Rmd’
--- re-building ‘Comprehensive-Housing-and-Affordability-Strategy.Rmd’ using rmarkdown
--- finished re-building ‘Comprehensive-Housing-and-Affordability-Strategy.Rmd’
--- re-building ‘Crosswalk.Rmd’ using rmarkdown
Quitting from lines 101-105 (Crosswalk.Rmd)
Error: processing vignette 'Crosswalk.Rmd' failed with diagnostics:
Did you forget to set the key? Please go to https://www.huduser.gov/hudapi/public/register?comingfrom=1 to and sign up and get a token. Then save this to your environment using Sys.setenv('HUD_KEY' = YOUR_KEY)
--- failed re-building ‘Crosswalk.Rmd’
--- re-building ‘Fair-Markets-Rent.Rmd’ using rmarkdown
--- finished re-building ‘Fair-Markets-Rent.Rmd’
--- re-building ‘Income-Limits.Rmd’ using rmarkdown
--- finished re-building ‘Income-Limits.Rmd’
--- re-building ‘Setup.Rmd’ using rmarkdown
--- finished re-building ‘Setup.Rmd’
SUMMARY: processing the following file failed:
‘Crosswalk.Rmd’
Error: Vignette re-building failed.
Execution halted
Static code analyses with lintr
lintr found the following 351 potential issues:
| message | number of times |
|---|---|
| Avoid 1:length(...) expressions, use seq_len. | 10 |
| Avoid 1:nrow(...) expressions, use seq_len. | 2 |
| Avoid using sapply, consider vapply instead, that's type safe | 1 |
| Lines should not be more than 80 characters. | 306 |
| Use <-, not =, for assignment. | 32 |
Package Versions
| package | version |
|---|---|
| pkgstats | 0.0.3.96 |
| pkgcheck | 0.0.2.276 |
Editor-in-Chief Instructions:
Processing may not proceed until the items marked with ✖️ have been resolved.
|
Dear @etam4260, |
|
Hi @jooolia An easy solution to deal with the naming conflict I think would be to just rename the entire package as ‘rhud’ and update the documentation accordingly. Otherwise, I have already contacted CRAN about this issue, but have not heard back about any resolutions. 😔 I’m not sure about any other options. Any other suggestions are appreciated. 🙏🏼 I took a look at the hudr package published on CRAN and it looks like they only implement APIs for two main datasets provided by HUD and they don’t have all the sub datasets. For example, Fair Markets Rent has state, county, and small area level data — they only have state level data. 🧐 hudr(CRAN)
They also provide functions for HUD miscellaneous APIs:
Single query means their function calls only make 1 API call at a time, whereas multi query means multiple API calls can be done in a single function. My package on the other hand supports the Crosswalk API and Comprehensive Housing and Affordability Strategy API as well. hudr(etam4260/hudr)
My package I believe is more flexible and intuitive: There are many ways of identifying US states such as using their abbreviation, full name, or fips code. My package allows the user to query based either of the options whereas the CRAN version strictly requires using the state abbreviation. For example, get_hud_fmr_statedata(entityid = “Virginia”, year = “2020”, hud_key = “dqwqdqwd”) does not work because entityid must be strictly ‘VA' Furthermore, their function arguments are less intuitive. For example: get_hud_fmr_statedata(entityid = “VA”, year = “2020”, hud_key = “dqwqdqwd”) 'entityid' I believe should be better named as state because you are only allowed to query for states. Furthermore, the function name itself I believe is a little verbose. My function names do not need the additionaly ‘get_’ at the front. ‘hud_key’ argument I believe should be better named as ‘key’. When specifying the year argument as well as arguments that are made of numbers, they don’t allow both character and numeric type to be used. For example, 11023 which can be represented as both a numeric or character can only be inputted as “11023”. I believe the flexibility could be both a good and a bad thing though. Their functions also only strictly allow for only singular API calls. For example, you cannot query: get_hud_fmr_statedata(entityid = c(“VA”, “CA"), year = “2020”, hud_key = “dqwqdqwd”) get_hud_fmr_statedata(entityid = c(“VA”), year = c(“2020”, “2019"), hud_key = “dqwqdqwd”) My implementation allows all of them in one function call as well allowing for multiple state and year inputs. hud_fmr(“VA”, year = 2020, key = “qdqdqdw”) hud_fmr(c(“VA”, “CA”), year = c(“2020”, “2019"), key = “dqdqdq”) hud_fmr(010099999, year = “2020”, key = “qdqdqdqq”) hud_fmr(‘METRO47900M47900’, year = 2020, key = “uefjhqiufd”) For the X marks on the pkgcheck: I have updated the repository to have a contributing file. Right now I have fixed rcmdcheck() so that it only give a single warning — a problem with rendering PDF documents with latex I think. It says something is wrong with Rd files. I’m not sure how to deal with the coverage error, as on my end codecov seems to be working fine. I made sure the HUD_KEY secret needed by the package is not "". If so, the tests and or code blocks are not run. 🤥 |
|
@ropensci-review-bot check package |
|
Thanks, about to send the query. |
|
🚀 Editor check started 👋 |
Checks for hudr (v0.1.0.9000)git hash: 120ebe8d
Important: All failing checks above must be addressed prior to proceeding Package License: GPL (>= 2) 1. Statistical PropertiesThis package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing. Details of statistical properties (click to open)
The package has:
Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages
All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by the The final measure (
1a. Network visualisationClick to see the interactive network visualisation of calls between objects in package 2.
|
| name | conclusion | sha | date |
|---|---|---|---|
| pages build and deployment | success | 120ebe | 2022-04-01 |
| R-CMD-check | success | 120ebe | 2022-04-01 |
| test-coverage | success | 06b739 | 2022-03-28 |
3b. goodpractice results
R CMD check with rcmdcheck
R CMD check generated the following error:
- checking tests ...
Running ‘testthat.R’
ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
══ Failed tests ════════════════════════════════════════════════════════════════
── Error (test-test_cdbgdr.R:2:3): Main CDBG-DR data files from DRGR Public Portal ──
Error incurl::curl_fetch_memory(file): Timeout was reached: [drgr.hud.gov] Resolving timed out after 10000 milliseconds
Backtrace:
▆- └─hudr::hud_cdbg(1) at test-test_cdbgdr.R:2:2
- ├─base::suppressMessages(import("https://drgr.hud.gov/public/downloads/DR-CDBG/CDBG-DR%20Financial%20Report%20by%20Appropriation.xlsx"))
- │ └─base::withCallingHandlers(...)
- └─rio::import("https://drgr.hud.gov/public/downloads/DR-CDBG/CDBG-DR%20Financial%20Report%20by%20Appropriation.xlsx")
-
└─rio:::remote_to_local(file, format = format) -
└─curl::curl_fetch_memory(file)
[ FAIL 1 | WARN 0 | SKIP 35 | PASS 2 ]
Error: Test failures
Execution halted
R CMD check generated the following notes:
- checking top-level files ... NOTE
File
LICENSE
is not mentioned in the DESCRIPTION file. - checking dependencies in R code ... NOTE
Namespace in Imports field not imported from: ‘devtools’
All declared Imports should be used.
R CMD check generated the following test_fail:
-
library(testthat)
library(hudr)
test_check("hudr")
[ FAIL 1 | WARN 0 | SKIP 35 | PASS 2 ]
══ Skipped tests ═══════════════════════════════════════════════════════════════
• Sys.getenv("HUD_KEY") == "" is TRUE (35)
══ Failed tests ════════════════════════════════════════════════════════════════
── Error (test-test_cdbgdr.R:2:3): Main CDBG-DR data files from DRGR Public Portal ──
Error in curl::curl_fetch_memory(file): Timeout was reached: [drgr.hud.gov] Resolving timed out after 10000 milliseconds
Backtrace:
▆
- └─hudr::hud_cdbg(1) at test-test_cdbgdr.R:2:2
- ├─base::suppressMessages(import("https://drgr.hud.gov/public/downloads/DR-CDBG/CDBG-DR%20Financial%20Report%20by%20Appropriation.xlsx"))
- │ └─base::withCallingHandlers(...)
- └─rio::import("https://drgr.hud.gov/public/downloads/DR-CDBG/CDBG-DR%20Financial%20Report%20by%20Appropriation.xlsx")
-
└─rio:::remote_to_local(file, format = format) -
└─curl::curl_fetch_memory(file)
[ FAIL 1 | WARN 0 | SKIP 35 | PASS 2 ]
Error: Test failures
Execution halted
R CMD check generated the following check_fails:
- no_import_package_as_a_whole
- rcmdcheck_stale_license_file
- rcmdcheck_imports_not_imported_from
- rcmdcheck_tests_pass
Test coverage with covr
ERROR: Test Coverage Failed
Cyclocomplexity with cyclocomp
The following functions have cyclocomplexity >= 15:
| function | cyclocomplexity |
|---|---|
| hud_chas | 27 |
| hud_cw | 23 |
| fmr_il_input_check_cleansing | 19 |
| cw_input_check_cleansing | 17 |
| hud_counties | 17 |
| hud_minor_civil_divisions | 17 |
| hud_places | 17 |
Static code analyses with lintr
lintr found the following 351 potential issues:
| message | number of times |
|---|---|
| Avoid 1:length(...) expressions, use seq_len. | 10 |
| Avoid 1:nrow(...) expressions, use seq_len. | 2 |
| Avoid using sapply, consider vapply instead, that's type safe | 1 |
| Lines should not be more than 80 characters. | 306 |
| Use <-, not =, for assignment. | 32 |
Package Versions
| package | version |
|---|---|
| pkgstats | 0.0.3.96 |
| pkgcheck | 0.0.2.276 |
Editor-in-Chief Instructions:
Processing may not proceed until the items marked with ✖️ have been resolved.
|
@ropensci-review-bot assign @jhollist as editor |
|
Assigned! @jhollist is now the editor |
|
@etam4260 I will be serving as the editor on this submission. I think this is a good fit and your explanation of how your package is different than the existing So, before I move on to assigning reviewers we need to get several items resolved:
If you have questions on any of this let me know. |
|
Just pinging you again @etam4260. Any questions about the few items I listed above? |
|
Hi @jhollist, Apologize for not getting back sooner: just nabbed some time to work on it a bit today. I have been working on fixing a few bugs I found on my end and creating more robust testing infrastructure. I have also changed the structure of some of the output data. Therefore, the documentation is a bit outdated -- which I will address. As for the failing test, I was able to fix that. I am hoping to get to the lintr issues soon as well as updating the name of the package (and documentation associated with it). As for one of the NOTES, it seems like its having a problem with files LICENSE.md, CODE_OF_CONDUCT.md and codemeta.json on the top level folder -- is there another location where these should be located? |
|
Couple of thoughts. As for the license stuff. You will need to make sure that DESCRIPTION knows that you have an additional file. If you add "+ file LICENSE" to "License: GPL (>= 2)" that should take care of that note. I believe the two other files just need to be added to .Rbuildignore. The root folder is a good spot for both (might event be required for codemeta.json). And no need to apologize! I understand the need to juggle multiple demands on time! |
|
Thanks for tip! I was able to deal with all the notes. For adding the "+ file LICENSE" to the description file, it gave me another error: "License components with restrictions not permitted." So I instead added the LICENSE file to the .Rbuildignore instead and it 'fixed' it. As for the lintr issues, I fixed maybe 97% of the issues -- most of the remaining ones deal with cyclocomplexity as well as function name length. I haven't applied those 'goodpractices' in the vignettes yet: it looks like lintr doesn't check those? Just need to update the package name and the associated documentation now! |
|
I think that sounds pretty good. Once you have the name and
documentation updated, let me know. I will start looking for reviewers
next week.
…On Fri, Apr 29, 2022 at 5:42 PM Emmet Tam ***@***.***> wrote:
@jhollist <https://github.com/jhollist>
Thanks for tip! I was able to deal with all the notes. For adding the "+
file LICENSE" to the description file, it gave me another error: "License
components with restrictions not permitted." So I instead added the LICENSE
file to the .Rbuildignore instead and it 'fixed' it. As for the lintr
issues, I fixed maybe 97% of the issues -- most of the remaining ones deal
with cyclocomplexity as well as function name length. I haven't applied
those 'goodpractices' in the vignettes yet: it looks like lintr doesn't
check those? Just need to update the package name and the associated
documentation now!
—
Reply to this email directly, view it on GitHub
<#524 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJPYS3F7XN2XGEGJ76D5VLVHRJTRANCNFSM5SHUCVZA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Jeffrey W. Hollister
email: ***@***.***
cell: 401 556 4087
https://jwhollister.com
|
|
Package has been renamed and documentation has been updated. 🙃 |
jhollist
changed the title
|
@ropensci-review-bot check package |
|
Thanks, about to send the query. |
|
🚀 Editor check started 👋 |
Checks for rhud (v0.2.0.9000)git hash: b5aa3d80
Important: All failing checks above must be addressed prior to proceeding Package License: GPL (>= 2) 1. Package DependenciesDetails of Package Dependency Usage (click to open)
The table below tallies all function calls to all packages ('ncalls'), both internal (r-base + recommended, along with the package itself), and external (imported and suggested packages). 'NA' values indicate packages to which no identified calls to R functions could be found. Note that these results are generated by an automated code-tagging system which may not be entirely accurate.
Click below for tallies of functions used in each package. Locations of each call within this package may be generated locally by running 's <- pkgstats::pkgstats(<path/to/repo>)', and examining the 'external_calls' table. basepaste (41), args (32), c (26), expand.grid (21), try (20), for (19), seq_len (19), as.data.frame (16), format (16), Sys.Date (16), call (14), length (13), do.call (10), as.character (6), grepl (6), nrow (6), unlist (6), rbind (5), as.integer (4), data.frame (4), Sys.getenv (4), cbind (3), rep (3), url (3), list (2), return (2), strsplit (2), substr (2), diff (1), emptyenv (1), grep (1), merge (1), nchar (1), ncol (1), Negate (1), new.env (1), readLines (1), regexec (1) rhudcw_input_check_cleansing (15), fmr_il_input_check_cleansing (7), chas_input_check_cleansing (6), chas_do_query_calls (4), misc_do_query_call (4), hud_fmr_state_counties (3), crosswalk_a_dataset_input_check_cleansing (2), hud_cw_zip_county (2), hud_fmr_state_metroareas (2), hud_state_minor_civil_divisions (2), hud_state_places (2), add_leading_zeros (1), capitalize (1), check_is_not_list (1), crosswalk (1), cw_do_query_calls (1), decimal_num (1), download_bar (1), elevennumbers (1), fivenumbers (1), fivenumsthenfournums (1), fix_geoid (1), fournumbers (1), hud_chas (1), hud_chas_county (1), hud_chas_nation (1), hud_chas_state (1), hud_chas_state_mcd (1), hud_chas_state_place (1), hud_cw (1), hud_cw_cbsa_zip (1), hud_cw_cbsadiv_zip (1), hud_cw_cd_zip (1), hud_cw_county_zip (1), hud_cw_countysub_zip (1), hud_cw_tract_zip (1), hud_cw_zip_cbsa (1), hud_cw_zip_cbsadiv (1), hud_cw_zip_cd (1), hud_cw_zip_countysub (1), hud_cw_zip_tract (1), hud_fmr (1), hud_fmr_county_zip (1), hud_fmr_metroarea_zip (1), hud_get_key (1), hud_il (1), hud_nation_states_territories (1), hud_set_key (1), hud_state_counties (1), hud_state_metropolitan (1), is.negative (1), numbers_only (1) utilszip (18), data (6) 2. Statistical PropertiesThis package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing. Details of statistical properties (click to open)
The package has:
Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages
All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by the The final measure (
2a. Network visualisationClick to see the interactive network visualisation of calls between objects in package 3.
|
| name | conclusion | sha | date |
|---|---|---|---|
| pages build and deployment | success | b5aa3d | 2022-05-03 |
| R-CMD-check | success | 3e3540 | 2022-05-03 |
| test-coverage | failure | 0b6873 | 2022-05-02 |
3b. goodpractice results
R CMD check with rcmdcheck
R CMD check generated the following notes:
- checking dependencies in R code ... NOTE
Namespaces in Imports field not imported from:
‘rio’ ‘zoo’
All declared Imports should be used. - checking R code for possible problems ... NOTE
crosswalk: no visible binding for global variable ‘w_geoid’
hud_state_minor_civil_divisions: no visible binding for global variable
‘FAlSE’
Undefined global functions or variables:
FAlSE w_geoid
R CMD check generated the following check_fails:
- cyclocomp
- no_import_package_as_a_whole
- rcmdcheck_imports_not_imported_from
- rcmdcheck_undefined_globals
Test coverage with covr
Package coverage: 0.74
The following files are not completely covered by tests:
| file | coverage |
|---|---|
| R/hudchas.R | 0% |
| R/hudcheckfornumbersonly.R | 0% |
| R/hudcheckisnotlist.R | 0% |
| R/hudcw.R | 0% |
| R/huddatasetcw.R | 0% |
| R/huddoquerycalls.R | 0% |
| R/hudfmr.R | 0% |
| R/hudinputcheckcleansing.R | 6.99% |
| R/hudkey.R | 0% |
| R/hudloadingbar.R | 0% |
| R/hudmisc.R | 0% |
| R/hudmischelpers.R | 0% |
| R/huduser.R | 0.48% |
| R/hudwebsitedocs.R | 0% |
Cyclocomplexity with cyclocomp
The following functions have cyclocomplexity >= 15:
| function | cyclocomplexity |
|---|---|
| crosswalk | 67 |
| crosswalk_a_dataset_input_check_cleansing | 50 |
| hud_chas | 28 |
| hud_cw | 25 |
| check_is_not_list | 20 |
| fmr_il_input_check_cleansing | 19 |
| hud_state_metropolitan | 19 |
| cw_input_check_cleansing | 17 |
| hud_state_counties | 17 |
| hud_state_minor_civil_divisions | 17 |
| hud_state_places | 17 |
Static code analyses with lintr
lintr found the following 17 potential issues:
| message | number of times |
|---|---|
| Avoid using sapply, consider vapply instead, that's type safe | 6 |
| Lines should not be more than 80 characters. | 10 |
| Use <-, not =, for assignment. | 1 |
Package Versions
| package | version |
|---|---|
| pkgstats | 0.0.4.30 |
| pkgcheck | 0.0.3.15 |
Editor-in-Chief Instructions:
Processing may not proceed until the items marked with ✖️ have been resolved.
|
@etam4260 Two things came up in the most recent checks 1.) A couple of functions are missing examples. Add these examples into the docs I think we are close enough. I will start looking for potential reviewers! |
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
Estimated hours spent reviewing: 8
Review CommentsGood job! I enjoyed going through your package and learning more about this API. On the whole, the package seems useful and I am glad to see that it will make it easier to interact the the HUD API. Thank you for inviting me to offer my review and critique. From the version I reviewed (84c8d), there are areas I suggest improving prior to publication with ROpenSci. This includes opportunities to:
Below is a detailed list of suggestions-- some of which you may already have addressed or thought about. It is a lot to go through, so feel free to disagree and take only what you find valuable. If there are elements that you agree with but which are ambitious, some can be tasked for future development. Docs and Communication
Tests
Code
Other code:
Miscellaneous
|
|
Great to see all this progress on the review! And apologies for not responding sooner I was out of town for a while and brought back COVID as a souvenir. I am much better now and trying to dig myself out. First, @rtaph and @khueyama thank you for two excellent reviews! Volunteer reviewers like you keep rOpenSci humming along! Second, @etam4260 lot's to consider in the reviews. Take a close look and start working on your changes and response. Regarding your question on CRAN submission (again please accept my apologies for the delay). I think a lot depends on how long you think it will take to get the revisions made. If only a couple of weeks, that you are probably good waiting until after revisions are done. If it will be longer than that, you can submit to CRAN now and then after revisions are completed and the package is accepted you can submit an updated version at that time. Biggest consideration is time between CRAN submissions. Best not to submit to frequently. Any questions for me or the reviewers, put those questions in this issue. |
|
@ropensci-review-bot submit review #524 (comment) time 4 |
|
Logged review for khueyama (hours: 4) |
|
@ropensci-review-bot submit review #524 (comment) time 8 |
|
Logged review for rtaph (hours: 8) |
ropensci-review-bot
added
4/review(s)-in-awaiting-changes
and removed
3/reviewer(s)-assigned
labels
|
Hi @khueyama
For your comment about the contributing file, this was not autogenerated from roxygen. I actually cookie cuttered it out of a more popular package, likely ggplot. I'm not exactly sure what else you would need in the CONTRIBUTING.md file. There are also issue templates for bugs and new features.
I have not worked on making tests for the internal functions yet -- will put that into my backlog. I must have confused "Focus on testing the external interface to your functions - if you test the internal interface, then it’s harder to change the implementation in the future because as well as modifying the code, you’ll also need to update all the tests." mentioned in the testing section of R packages: https://r-pkgs.org/tests.html#what-to-test
I added code blocks for helping the user set the key, how to type a simple example, and also options for tibbles vs data frames in the README.md.
I added documentation in the '@return' roxygen for most functions to mention that it will query a combination of geoid, year ...
I think I fixed this bug. Now it should add a new line before and after adding Sys.setenv() to the .Rprofile as well as making sure to append to the home directory .Rprofile instead of that in the working directory.
I mentioned this to my advisor and since most my work completely overlaps the hudr work, including more, there likely won't be much to gain in terms of merging them together.
I have the package byte compiled when the user installs it. With my understanding of compilers, they tend to optimize away inefficiencies common in for loops. In this case, I'm thinking appending items to vectors isn't too much of a problem because of this, but I have yet to benchmark anything to confirm it. I do see the concerns with readability -- will try to think of a non-for loop implementation.
I have removed this file as well as some other functions in other files that were not being used.
I agree with you. My initial approach was that I didn't want to abstract too many things away, just in case I didn't implement them in a way which would make it effectively reusable. I think this has to do with the lack of foresight to know what pieces of code I might reuse. I will definitely work on this.
I 'partially' fixed this issue. I now have an interface for the user to set a user-agent. However, I defaulted it to the github repo. I think if a user wanted to use it for malicious means, then they could easily set the user agent to any other url to try and disguise themselves as someone or something else. I am hoping that using the github url might allow HUD IT to quickly pinpoint the problem and mitigate it. If they do ban the specific user agent, then a 'good user' could just set it to a different one.
I think these should be fixed now. Although, now I am getting warnings about size capacity from check. I think I might need to move the pkgdown website to a different repository.
I think there were just some missing assert warning in some of the tests. These should now be added in.
Agree. File names in R/ now include the underscore between words. I haven't read through the tidyverse style guide, but looks interesting: will take some time to go through it. Are there naming conventions for vignettes and test files? |
|
Hi @rtaph I've begun working on some of the feedback you made. Here is where I am so far...
This should be fixed. However there is an issue with cbsadiv queries for the USPS Crosswalk files. I think this might be a bug on HUD USER's end.
I'm not exactly sure what else needs to go in the CONTRIBUTING file. The Zenodo badge is now added to the readme and the CITATION file should now have the Zenodo bibentry.
I am currently working on this.
The ROpenSci package guidance says The README, the top-level package docs, vignettes, websites, etc., should all have enough information at the beginning to get a high-level overview of the package and the services/data it connects to, and provide navigation to other relevant pieces of documentation. I don't think all vignettes yet provide an easy entry point. I would encourage adding to their text to make it clearer. In general I found the examples from the vignettes a bit hard to follow. There is an opportunity to improve them by adding some commentary on the output. This is probably not needed for all flavours of cross-walk, but commenting on the first example in your vignette could really help. I have someone else working on this: a geography student who will definitely explain it better than me. As for the vignettes, I'm not sure what else would make the vignettes clearer as I mention the data source and the reason why the data source exists. I then provide examples for how to get the data. As for adding commentary, is explaining the columns in each dataset in the context of an output make sense for each vignette?
Agree -- though I have this in the backlog as I feel like this is very minor usability issue.
I attempted to do this. However, I did run into some trouble getting pkgdown to parse the pkgdown.yml file correctly. So I might attempt this again some other time.
Working a bit on this. I will opt for simple titles such as:
I agree. I did some major reorganizing in the README: still need to move the citation information down to the bottom.
I actually attempted to use the @family, but the way they organized the family functions in the documentation was very "mushed together". I think the '@Seealso' allowed me to customize how each member was organized in relation to another.
Although not required, it would be useful to have an @example for internal functions. This would be helpful for reviewers or future code maintainers even if you have @nord. Agree. I think this could be generalized to all internal functions I have in the package. I spent most of my time on ensuring the quality of the exported functions documentation and left the internals docs for later.
This should no longer be relevant. I don't mention data types or classes in the vignette parameters section anymore.
I wished R had some type hinting like that available in python: definitely would make it easier to document that. As for adding type hints to the '@param' I agree. In (most if not all) I have defaulted to accepting vectors only.
Agree. Haven't made the changes yet, but will put that in my backlog.
Agree. I have someone else working on this.
I have removed all instances of a fake key in the function examples.
I would review the name of the unit tests to make them conform to an imperative tense. For example, "test awkward crosswalk" could be changed to "an improper crosswalk request must error" so that it plays nice with the phrase "test that ...""" Agree : these are in the backlog.
Agree: this is in the backlog.
Two tests fail rather than skip when the API key is missing. """ The test failing when API key is fixed locally. However, as of right now, I have not pushed the changes to the repo. As for web mocking and scheduled workflows, this is something I have far back in the backlog. Didn't want to spend too much time on web-mocking if I didn't know if my implementation was working. These are my thoughts so far. As for the: (Code, Other Code, and Miscellaneous) sections mentioned in your review, I will put that in another comment as this one seems to be getting very long. And thanks again for such a comprehensive review. Definitely a lot to learn about! |
|
Hi @etam4260 Great to see so much activity!
IMO the elements you have in the CONTRIBUTING file are sufficient. I think I just saw "This outlines how to propose changes to rhud..." and thought the ellipses indicated this might be an unfinished sentence.
I have two suggestions:
|
|
All, sorry for the long delay in checking in on this. @etam4260 have you had a chance to look at the last few comments from @rtaph? When you have, let me know. At that point, @rtaph and @khueyama, I will ask for for you to take a close look at the revisions and get your agreement that the package has been revised to your satisfaction. Hope you all are having a good summer (and are staying cool!) |
|
Hi, @rtaph @jhollist @khueyama
Furthermore, I know the use of RETRY was mentioned in one of the reviews. This is something getting worked on, but I am assuming this should include a getOption for the number of retries that a user would want. @rtaph as for your suggestion on printing the output from the example, do you mean using print() to show in console? Also do you recommend separating the package from the website documentation as it looks like it is throwing NOTEs related to the /doc folder size? Otherwise are there any other issues? |
|
Thanks for the update, @etam4260 ! It is totally reasonable to leave some suggestions for future consideration. I will try to carve out time this weekend to look at your latest changes. In the meantime:
|
@rtaph You don't really need to worry, because as soon as the package is transferred the docs will be built by the internal rOpenSci doc server, and you can just remove the whole /docs folder anyway. See the Packaging chapter of Dev Guide for details. |
|
Sorry for delay in responding. I am on vacation this week and next and mostly away from my computer so I might be a little slow in responding to this. I will get to it as soon as I am able. And thanks @mpadge for the assist! |
|
Thanks for the info @mpadge and good to know. Do you know if this is the same case for CRAN? @rtaph Additional info: I think my testing suite is still a little bit incomplete as of now. I need to go back and make sure I test exact outputs versus determining if I get some output. In my case, the API for USPS Crosswalk API changed, but my tests didn't catch that. Also not at that 75% test coverage mark just yet 😅 |
|
Okay, no problem! I'll hold off on reviewing the code then, until you let me know you think |
|
@etam4260 Just now getting back to this! CRAN won't see your docs folder as you currently have it listed in your .Rbuildignore file. That is the way you want it. The documentation website will be independent of anything that happens on CRAN. I think I would follow @mpadge advice and remove the docs folder. rOpenSci will take care of building that for you. At least that is what I can tell from digging around a bit. I haven't used pkdgown sites much myself so don't have a ton of personal experience here. |
|
@etam4260 just checking in to see how you are making out on the edits. |
|
@etam4260 Hoping to get this submission wrapped up. Please let me know where things stand. Thanks! |
|
@ropensci-review-bot put on hold |
|
Submission on hold! |
|
@etam4260 Just giving a heads up that we have put this submission on hold. Will check in again in 3 months unless I hear from you before that. |
|
@jhollist: Please review the holding status |
|
@etam4260 Any updates on rhud? Have you made progress on edits? Do you expect too in the next couple of months? |
Submitting Author Name: Name
Due date for @rtaph: 2022-06-07Submitting Author Github Handle: @etam4260
Other Package Authors Github handles: (comma separated, delete if none)
Repository: https://github.com/etam4260/rhud
Version submitted:
Submission type: Standard
Editor: @jhollist
Reviewers: @rtaph, @khueyama
Due date for @khueyama: 2022-06-09
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):
It is a data retrieval package because it retrieves data from an API. It 'will' be a data munging package after implementation of additional features such as cross walking an entire dataset. Furthermore, the APIs which this package retrieves data from are associated with geographic identifiers.
I am hoping to reach professors, researchers, and students with this package. This gives access to the crosswalk files which is a geospatial technique described very well in these journal articles:
Din, Alexander and Wilson, Ron, 2020. “Crosswalking ZIP Codes to Census Geographies: Geoprocessing the U.S. Department of Housing & Urban Development’s ZIP Code Crosswalk Files,” Cityscape: A Journal of Policy Development and Research, Volume 22, Number 1, https://www.huduser.gov/portal/periodicals/cityscpe/vol22num1/ch12.pdf
Wilson, Ron and Din, Alexander, 2018. “Understanding and Enhancing the U.S. Department of Housing and Urban Development’s ZIP Code Crosswalk Files,” Cityscape: A Journal of Policy Development and Research, Volume 20 Number 2, 277 – 294.
Additionally, it provides access to Income Limits and Fair Markets Rent as well as Comprehensive Housing and Affordability datasets provided by HUD which is of interest to housing and social science researchers.
Implementation of a crosswalk function is planned in future releases, which will help crosswalk a US dataset from one geographic identifier into another using the method described in the papers above.
Recently, a hudr package got published on CRAN, but it looks like some of derives from the work I currently have. I am not sure how this will affect my prospects of submitting this to CRAN. Furthermore, their package provides only access to the fair markets rent and income limits API provide by HUD. Mine gives access to all the APIs that are currently supported by HUD USER (https://www.huduser.gov/portal/home.html) as well as providing more flexibility and intuitiveness.
As for documentation and testing, I believe my package could be improved. I don't test very many edge cases and have not created vignettes for every function.
For the most part, I think yes. The package requires an API key which I have users store using Sys.setenv(). I have not looked into the more sophisticated methods like the keyring package and do not instruct the user on how to set the key to be persistent.
#500
@jooolia
pkgcheckitems which your package is unable to pass.I seem to get some errors when running pkgcheck. I manually made sure I had all the necessary components.
For the pkgcheck requirement that says all functions need examples, I am assuming that only includes exported ones?
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
Code of conduct
The text was updated successfully, but these errors were encountered: