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

Change get_sotkanet default years, add function sotkanet_indicators_metadata and move from httr to httr2 #24

Merged
merged 17 commits into from
Jun 13, 2024

Conversation

Allaht2
Copy link
Contributor

@Allaht2 Allaht2 commented May 27, 2024

Change get_sotkanet years parameter default from 1991:2015 to NULL. The default now gives the data from all the available years. Also update the function documentation and add more examples.
Add function sotkanet_indicators_metadata and update other functions to use it instead of SotkanetIndicatorsMetadata.

@Allaht2 Allaht2 requested a review from pitkant May 27, 2024 08:42
@Allaht2 Allaht2 changed the title Change get_sotkanet default years and add function sotkanet_indicators_metadata Change get_sotkanet default years, add function sotkanet_indicators_metadata and move from httr to httr2 May 28, 2024
@Allaht2
Copy link
Contributor Author

Allaht2 commented May 28, 2024

Move from httr to httr2. All the old and new functions that used httr functions now use httr2 functions instead.

@pitkant pitkant mentioned this pull request May 28, 2024
This was linked to issues May 28, 2024
Copy link
Member

@pitkant pitkant left a comment

Choose a reason for hiding this comment

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

Good work

path <- paste(url_object$path, sotkanet_uri, sep = "")
url_object$path <- path
final_url <- httr::build_url(url_object)
final_url <- httr2::url_build(url_object)

res <- sotkanet.json_query(final_url,
Copy link
Member

Choose a reason for hiding this comment

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

Here flatten = TRUE but in sotkanet.json_query function the parameters is not passed forward anymore (?)

R/json.R Outdated
Comment on lines 34 to 36
response <- httr2::request(url) %>% httr2::req_user_agent(useragent) %>%
httr2::req_perform() %>% httr2::resp_body_json(simplifyVector = TRUE)

Copy link
Member

Choose a reason for hiding this comment

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

Is the output still the same as before? Is simplifyVector essentially the same as flatten = TRUE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that simplifyVector does work in a similar way. At least in my testing the output is similar with simplifyVector.

R/get_sotkanet.R Outdated
Comment on lines 25 to 26
#' work with only the gender value 'total' and return an empty data.frame. In these situations
#' it is advised to check out the [eurostat] package instead.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove eurostat reference and instruct user to just try with other options

@pitkant
Copy link
Member

pitkant commented Jun 10, 2024

Maybe one final addition: since packages like kableExtra and ggplot2 are only in Suggests and not Depends, their presence/absence could be used to control the evaluation of vignette building? Like this:

eval = requireNamespace("somedependency")
eval = !identical(Sys.getenv("SOME_THING_YOU_NEED"), "")
eval = file.exists("credentials-you-need")

See Vignettes - Special considerations for vignette code for more info.

@Allaht2 Allaht2 merged commit bb6581e into v0.10-dev Jun 13, 2024
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.

Switch from httr to httr2 Default value for years
2 participants