Skip to content
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

Changes to documentation as a result of review #73

Closed
wants to merge 4 commits into from

Conversation

potterzot
Copy link

These changes primarily address some minor issues around the reporter, partner, and partner_2 parameters in ct_get_data(). It looks like in the process of implementing the convenience value of all, some documentation was not updated to reflect that defaults in some cases are now World, that all is an option, and that All is not a valid option.

datapumpernickel and others added 4 commits November 17, 2023 12:19
updates from review 1 by rOpenSci
fix error with goods vs. services classification of BEC codes

Setting these parameters to `NULL` will query all possible values related to the mode of transport, secondary partner, and customs procedures. This provides a comprehensive view of the data across different transportation modes and customs categories.
Using `NULL` for `partner` will query the aggregate `World` partner. Using `all` will query for all partners. This allows you to retrieve trade data for the world in aggregate or for all countries without specifying individual ISO3 codes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit tricky and I can see how it can be misunderstood! I would like to work with you on how I could make this more explicit.

What you did, was this:

comtradr::ct_get_data(reporter = "DEU", 
                      start_date = 2020, 
                      end_date = 2020)

You left the argument without any specification and it goes to the default of world (as specified in the help file, whihc says: 'Partner ISO3 code(s) or NULL. See comtradr::country_codes for possible values. Default: 'World' (all partners as an aggregate).'

However what I MEANT, when I wrote setting it to NULL, was:

``` r
comtradr::ct_get_data(reporter = "DEU", 
                      partner = NULL, 
                      start_date = 2020, 
                      end_date = 2020)

Then, it will actually query all possible values.

I am not saying that you are wrong, but the way you have put it here, also is not correct, because using NULL will actually query all parameters. Not sure how to make the distinction between NULL and just not specifying the argument.

Do you have a suggestion?

Copy link
Contributor

@datapumpernickel datapumpernickel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @potterzot

Thank you so much for this thorough review, your time and the helpful comments!
I will make an issue with the requested changes and will later this week comment in the thread. But since the pull request is already here and overall helpful, I thought I could include it already, but there seems to be a misunderstanding with setting the argument to NULL and leaving it in blank. I have left a comment in the respective part ofthe code!

Looking forward to your input on this!

Thanks again!

@datapumpernickel datapumpernickel deleted the branch ropensci:dev December 23, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants