-
-
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
comtradr: API wrapper for UN Comtrade data #613
Comments
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type |
🚀 Editor check started 👋 |
Checks for comtradr (v0.4.0.0)git hash: e82abc78
Package License: GPL-3 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. basec (44), list (4), body (3), paste0 (3), colnames (2), data.frame (2), get (2), if (2), return (2), switch (2), as.numeric (1), by (1), length (1), message (1), names (1), new.env (1), paste (1), seq.Date (1), Sys.getenv (1), tolower (1), which (1), with (1) comtradrct_get_ref_table (8), check_date (2), comtrade_after (2), comtrade_error_body (2), comtrade_is_transient (2), ct_build_request (2), ct_check_params (2), ct_download_ref_table (2), ct_perform_request (2), ct_process_response (2), is_year (2), check_clCode (1), check_customsCode (1), check_flowCode (1), check_freq (1), check_motCode (1), check_partner2Code (1), check_partnerCode (1), check_reporterCode (1), check_type (1), convert_to_date (1), ct_commodity_db_type (1), ct_commodity_lookup (1), ct_country_lookup (1), ct_get_data (1), ct_get_remaining_hourly_queries (1), ct_get_reset_time (1), ct_register_token (1) clicli_inform (34), cli_warn (2) statsupdate (9), frequency (5), time (1) httr2resp_body_json (4), resp_header (4), request (2), req_error (1), req_headers (1), req_retry (1), req_throttle (1) graphicstext (7) methodsisGroup (4), new (1) purrrmap (2), map_chr (1), map_int (1) rlangarg_match (4) utilsdata (4) fspath_package (3) lubridateyear (2) stringrstr_extract (1) NOTE: Some imported packages appear to have no associated function calls; please ensure with author that these 'Imports' are listed appropriately. 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.
|
id | name | conclusion | sha | run_number | date |
---|---|---|---|---|---|
6522795714 | pkgcheck | success | e82abc | 5 | 2023-10-15 |
6522795715 | R-CMD-check | NA | e82abc | 50 | 2023-10-15 |
6522795709 | test-coverage | success | e82abc | 30 | 2023-10-15 |
3b. goodpractice
results
R CMD check
with rcmdcheck
R CMD check generated the following note:
- checking data for non-ASCII characters ... NOTE
Note: found 11 marked UTF-8 strings
R CMD check generated the following check_fail:
- rcmdcheck_non_ascii_characters_in_data
Test coverage with covr
Package coverage: 79.64
Cyclocomplexity with cyclocomp
No functions have cyclocomplexity >= 15
Static code analyses with lintr
lintr found the following 183 potential issues:
message | number of times |
---|---|
Avoid library() and require() calls in packages | 14 |
Lines should not be more than 80 characters. | 169 |
Package Versions
package | version |
---|---|
pkgstats | 0.1.3.9 |
pkgcheck | 0.1.2.9 |
Editor-in-Chief Instructions:
This package is in top shape and may be passed on to a handling editor
@ropensci-review-bot assign @noamross as editor |
Assigned! @noamross is now the editor |
@ropensci-review-bot seeking reviewers |
Please add this badge to the README of your package repository: [![Status at rOpenSci Software Peer Review](https://badges.ropensci.org/613_status.svg)](https://github.com/ropensci/software-review/issues/613) Furthermore, if your package does not have a NEWS.md file yet, please create one to capture the changes made during the review process. See https://devguide.ropensci.org/releasing.html#news |
Thanks @datapumpernickel for your submission (and your work taking over maintenance and bringing this package up to speed). As the bot says, this package is in nice shape, and I'll be seeking reviewers. |
Thank you, I am looking forward to the review and feedback! |
@ropensci-review-bot assign @ernestguevarra as reviewer |
@ernestguevarra added to the reviewers list. Review due date is 2023-11-06. Thanks @ernestguevarra for accepting to review! Please refer to our reviewer guide. rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more. |
@ernestguevarra: If you haven't done so, please fill this form for us to update our reviewers records. |
📆 @ernestguevarra you have 2 days left before the due date for your review (2023-11-06). |
Hi @datapumpernickel. Just a quick note to say that I am almost done with a comprehensive pass at reviewing your package! Sorry that I have been delayed. When I accepted the review role, I forgot where I should look to see the review process and my GitHub notifications were getting archived on my email. Otherwise, I will be posting my review notes here in the next couple of hours. |
Hi @ernestguevarra, no worries. You are just in time! Looking forward to the review. Thank you for the work to look into everything already! |
@ropensci-review-bot assign @potterzot as reviewer |
@potterzot added to the reviewers list. Review due date is 2023-11-27. Thanks @potterzot for accepting to review! Please refer to our reviewer guide. rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more. |
@potterzot: If you haven't done so, please fill this form for us to update our reviewers records. |
@datapumpernickel, 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
Estimated hours spent reviewing:
Review CommentsTest installationBoth local install and remote install (via GitHub) of the package proceeded without issues and as expected/documented. Check package integrity
── R CMD check results ──────────────────────────────────────────────────────────────────────────── comtradr 0.4.0.0 ────
Duration: 19.1s
0 errors ✔ | 0 warnings ✔ | 0 notes ✔
══ Results ══════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 2.0 s
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 78 ]
── GP comtradr ──────────────────────────────────────────────────────────────────────────────────────────────────────────
It is good practice to
✖ write unit tests for all functions, and all package code in general. 79% of code lines are
covered by test cases.
R/ct_build_request.R:52:NA
R/ct_check_params.R:296:NA
R/ct_check_params.R:300:NA
R/ct_check_params.R:301:NA
R/ct_check_params.R:302:NA
... and 97 more lines
✖ 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
data-raw/cmd_codes.R:7:81
data-raw/country_codes.R:7:81
data-raw/country_codes.R:22:81
data-raw/country_codes.R:53:81
data-raw/DATASET.R:11:81
... and 164 more lines
✖ fix this R CMD check NOTE: Note: found 11 marked UTF-8 strings
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Check package metadata files
WORD FOUND IN
️ README.md:49,102,124
README.Rmd:40,83,106
accomodate README.md:28
README.Rmd:30
arg ct_commodity_lookup.Rd:40
NEWS.md:26,56,57,58
comtradr.Rmd:98,194,211
args ct_commodity_lookup.Rd:45
NEWS.md:50
ASEAN country_codes.Rd:16
CMD README.md:10
README.Rmd:18
Codecov README.md:12
README.Rmd:19
comtrade ct_build_request.Rd:10
ct_perform_request.Rd:10,17
get_primary_comtrade_key.Rd:5,10
set_primary_comtrade_key.Rd:10,13
README.md:91
README.Rmd:75
comtradr.Rmd:61
Comtrade comtradr-package.Rd:7,11
country_codes.Rd:17,18,28
ct_commodity_lookup.Rd:5,52,54,57
ct_get_data.Rd:5,53,69,77
ct_perform_request.Rd:5,20
get_primary_comtrade_key.Rd:13
set_primary_comtrade_key.Rd:5,16
title:1
description:1,2
NEWS.md:29,54,77
README.md:15,16,24
README.Rmd:22,22,28
comtradr.Rmd:29,29,119,183,203,211
COMTRADE get_primary_comtrade_key.Rd:10,13
set_primary_comtrade_key.Rd:16
env get_primary_comtrade_key.Rd:13
set_primary_comtrade_key.Rd:16
func NEWS.md:26,32,34,36
github comtradr-package.Rd:33,34
https comtradr-package.Rd:33,34
httr ct_build_request.Rd:17,20
ct_get_data.Rd:47,66
ct_process_response.Rd:10,20
ℹ️ README.md:95
README.Rmd:77
comtradr.Rmd:63
iso ct_country_lookup.Rd:16
ct_process_response.Rd:20
json ct_perform_request.Rd:17
ct_process_response.Rd:20
NEWS.md:34
natively comtradr.Rmd:192
onboarding comtradr-package.Rd:33,34
ORCID comtradr-package.Rd:23,33,34
Readme NEWS.md:4
repo README.md:51
README.Rmd:42
ropensci comtradr-package.Rd:33,34
README.md:150
README.Rmd:130
rOpenSci comtradr-package.Rd:33,34
README.md:9
README.Rmd:17
stringified NEWS.md:36
testthat NEWS.md:83,99
twi ct_pretty_cols.Rd:9
useability README.md:36
README.Rmd:32
Check documentation
Test functionalityI have been using the new So, based on this experience since June, I can say that I've been able to push the A few things that maybe considered by the author/maintainer (probably as a new feature in upcoming updates) are:
|
@datapumpernickel you may notice that a big chunk of my review has been on the documentation. As I was doing the review, I forked your package and have been making edits on the docs/help files/vignettes as I was reviewing them. I am happy to make a pull request with my edits so you can have a look at what these changes may look like. Let me know if you think this would be helpful. Otherwise, thank you for your good work on getting |
Hi @ernestguevarra! first of all, thank you again for your thorough review and helpful comments - this is great! I am also really happy to hear that you were able to already put the package to use and found it useful.
|
@datapumpernickel I will finalise my edits today on the package (mainly documentation) and will make a PR to Also, you can just use the PR as a reference for what I am suggesting to change and you can implement the changes yourself as you update the package. I think sometimes that might be easier so you have full control of the changes. So, do with it as you deem best and most efficient for your workflow. |
👋 Hello @potterzot, just a friendly reminder your review is due. |
@ropensci-review-bot check package |
Logged review for potterzot (hours: 4) |
@ropensci-review-bot submit review #613 (comment) time 4 |
Logged review for ernestguevarra (hours: 4) |
Thanks for your review @potterzot! @datapumpernickel, now both reviews are in. Let us know when you've implemented and/or have a response to the requested changes |
Hi @noamross @ernestguevarra and @potterzot, thanks to all again for your valuable time and the thorough review! Your comments, additions and tests have already been of great help! I have made an issue with all the requested changes and am tracking which ones I have already implemented there: ropensci/comtradr#69 Some of the larger documentation tasks (new vignettes for transitioning from older iteration and on how to loop for bulk data) might take me a little longer, but some I might get done in the next week. I will report back once I am done with all the changes. Most I have no further comments on, as I find them sensible and helpful! @ernestguevarra Would you mind submitting that pull request you wanted to do? It would also be helpful to me, if you have any example code that led you to get 500 Errors! Thanks! There is two issues, where I think I could need some more input: Question 1Currently, I use the comtradr::ct_get_data(reporter = "DEU",
patner ='ARG',
start_date = 2020,
end_date = 2020) The API did not work as expected. Instead of returning partner Argentina, it returned partner "World" without an error, because the mistyped However, do you have any other idea on how to allow the passing on of arbitrary parameters to the function? e.g. an argument that takes a named list? I am a bit our of my depth here. Question 2 - Confusion around NULL, "all" and default values.As has been noted by @potterzot there is some confusion around this. Here is whats happening: Argument in Blankcomtradr::ct_get_data(reporter = "DEU",
start_date = 2020,
end_date = 2020) --> This returns Germanys trade with "World", as the argument is unspecified, it defaults to "World". Argument set to NULLcomtradr::ct_get_data(reporter = "DEU",
partner = NULL,
start_date = 2020,
end_date = 2020) --> This returns Germanys trade with all possible partners, notably, including World! Argument set to 'all'comtradr::ct_get_data(reporter = "DEU",
partner ='all',
start_date = 2020,
end_date = 2020) This returns Germanys trade with all partner countries that are not groups (e.g. ASEAN is not included, World is not included). This is what you would use, if you would want to aggregate the World values yourself. When I implemented this, it seemed clear to me, but I can now see how this confuses people. I am thinking about changing the default from "World" to NULL, because it is more intuitive in the documentation to think about leaving it in blank as similar to as setting it to NULL. However, for the parameters custom_code, and mode_of_transport, it is quite confusing, when all of a sudden for one trade relationship you get a bunch of weird entries with different modes of transport and custom codes, when in 99% of all cases, what you want is only the total (Data is realtively new in comtrade and not that reliable yet either). Hence the default "total". Do you have any ideas/suggestions on how to best solve this? Maybe I should just include it as clear as here in the documentation somewhere? Thank you again for your time, really highly appreciated! |
@noamross One more question for you: Currently, since this package has been reviewed in the past, there is two people who have reviewed the package in the past in the Contributors file. I would just add the two new reviewers to this list, right? We can also keep the old reviewers in, although technically, they have not reviewed the package as is, because we virtually re-wrote 90% of the functions. Not sure what is the policy here. If none, I would just keep everybody in the list. |
@datapumpernickel This a new one for us, but I would say yes, keep the previous reviewers, just as you retain the original author. Their contribution to the lineage of the package remains, even if much of the code they reviewed does not. |
Q1: I think an Q2: I agree that NULL is a common default value. If NULL is an option, I would have it be the default, and not an uncommonly used option. I would suggest (a) the default return smaller data, and (b) that all the options be equivalent. So in this case my suggestion is to retain 'world' as default and remove NULL as an option. Instead come up with another descriptive named option like |
@datapumpernickel just a couple of thoughts on Q1 and Q2:
Here are links to the above functions to see the code in entirety: expand_list(), parameter_is_valid(), nassqs_GET There is a function that helps with this that will just return all valid parameters and their description to help with usability as well.
In general, as a user I prefer a default behavior that fetches less data, so would not prefer the option of having a default that fetches all subcategory values. |
Hi @datapumpernickel. rOpenSci Community Manager here. If you want to join our Slack, I can send an invitation for you. Please let me know. You can write me to yabellini@ropensci.org |
Dear @ernestguevarra, @potterzot and @noamross, I have now implemented all changes to the best of my abilities. Thank you also for the helpful feedback on my additional questions. This has introduced a minor breaking change, as now I have basically kept the Defaults as restrictive as is, to not return too much data. There is no more NULL option, instead you can pass I have also gotten rid of Implemented changes are listed here: ropensci/comtradr#69 As a next step, I would publish to CRAN, I suppose. All the best and thank you again to everyone for the helpful comments and your time! @ropensci-review-bot submit response #613 (comment) |
Dear @yabellini, I am already on the Slack-channel, but thanks for the invite! |
@datapumpernickel: please post your response with Here's the author guide for response. https://devguide.ropensci.org/authors-guide.html |
Thanks you for your follow-up, @datapumpernickel! @potterzot and @ernestguevarra, please let us know if you agree the changes address the comments in your review. And Happy Holidays to all! |
@noamross I agree that changes address comments and think the package is ready. Thanks! |
@datapumpernickel sorry for dropping the ball on this. Thank you for all the progress achieved since my review. I agree with the changes made in relation to my review and also those that have been made in response to @potterzot and @noamross feedback. I've worked with the latest version a couple of days ago and specifically tried the functions to which changes have been made. All looks good and I agree that the package is ready! Really appreciate your efforts into this. I think a lot of people will benefit from this! |
@ropensci-review-bot approve comtradr |
Approved! Thanks @datapumpernickel for submitting and @ernestguevarra, @potterzot 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 post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @ropensci/blog-editors in your reply. They will get in touch about timing and can answer any questions. We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding (with advice on releases, package marketing, GitHub grooming); the guide also feature CRAN gotchas. Please tell us what could be improved. Last but not least, you can volunteer as a reviewer via filling a short form. |
Thank you @datapumpernickel for bringing this package to this point and putting it through review after taking over maintenance! Of course most of the steps above don't apply for a package already in our suite. I do think that the story of this package and review would make a great blog post. Taking over maintenance is a big thing to tackle and the more stories we have of it the better we are able to guide people through it! Please let us know if you are interested in writing one. |
Hi all! Really excited to see this package fully approved back in the fold of rOpenSci! Thanks to everybody for the input and your time. Currently I am waiting for some feedback on the caching ability I am implementing. Then I would prioritize getting the package on CRAN. As soon as that is done I will be in touch about writing up the process! :) |
Date accepted: 2024-01-09
Submitting Author Name: Paul Bochtler
Submitting Author Github Handle: @datapumpernickel
Repository: https://github.com/ropensci/comtradr
Version submitted: 0.4.0 (not yet released)
Submission type: Standard
Editor: @noamross
Reviewers: @ernestguevarra, @potterzot
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):
The package leverages httr2 to extract trade data from the UN Comtrade API, which is one of the standard sources for trade data world wide. See here for more info on the API.
Who is the target audience and what are scientific applications of this package?
The package can be used for econometric research into the trade relations of countries, dependencies between countries and many other use cases for trade data. The target audience are students and scientists, as well as practitioneers in the field of econometrics, political science among others.
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
As far as I am aware, there is no other package that wraps the UN Comtrade API.
(If applicable) Does your package comply with our guidance around Ethics, Data Privacy and Human Subjects Research?
Not applicable.
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.
The package has previouslz been reviewed by rOpenSci and is already part of the rOpenSci suite. However, since the API underwent fundamental changes, so did most of the functions of the package. Hence, we asked, whether we could get another review. This inquiry was answered positively here: ropensci/software-review-meta#100
pkgcheck
items which your package is unable to pass.As far as I am aware, the package currently passes all the pkgcheck checks.
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: