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

sc_table: Allow json request to be passed as string #36

Closed
wants to merge 1 commit into from

Conversation

matmo
Copy link

@matmo matmo commented Dec 15, 2022

Currently sc_table only allows json files to be used. This is a limitation for certain use cases (when you, e.g. want to dynamically update the query or want to bundle the json query with R code).

This commit is a quick fix that allows the user to provide a json query from an R object.

The implementation preserves the current behaviour: If provided, the json_file is read in an takes precedence over the new json parameter. If no json_file is provided the user-supplied json query is sent to the server instead.

@GregorDeCillia
Copy link
Contributor

Thank you! I am a little bit of a perfectionist when it comes to "naming things" so I'll have to think about how exactly this should be included in sc_table(). My current idea would be to "overload" the first argument and rename it from json_file to json.

#' @param json path to a json file, which was downloaded via the STATcube GUI ("Open Data API Query").
#'   Alternatively, a json string with a class attribute of `"json"`.
sc_table(json, language = NULL, add_totals = TRUE, key = NULL) {
  if (!inherits(json, "json"))
     json <- readLines(json, warn = FALSE)
  # ...
}

The recommended way for custom requests is to use sc_table_custom() which will soon be extended to allow recodes (see #33). But I totally understand that sometimes it is easier to modify a downloaded json request from the STATcube GUI and pass it as a string.

@matmo
Copy link
Author

matmo commented Dec 16, 2022

That sounds like a much more elegant solution indeed but might require some rewriting for the table class, since it involves uses the file= option.

Another alternative would be to just skip readLines() altogether and leave this job to fromJSON, since it parses both files and strings. We then just have to hand over the correct option (file or json) to the class init function (or move the validate step into the class itself so we can get rid of the ugly ifelse()s):

sc_table <- function(json, language = NULL, add_totals = TRUE,
                     key = NULL) {
  ...
  is_json <- jsonlite::validate(json)
  res <- sc_table_json_post(json, language, add_totals, key) %>%
    sc_table_class$new(json = ifelse(is_json), file = ifelse(is_json, ...), add_totals = add_totals)

GregorDeCillia added a commit that referenced this pull request Dec 16, 2022
reimplements #36 with a slightly
different approach in regards to
naming
@GregorDeCillia
Copy link
Contributor

GregorDeCillia commented Dec 16, 2022

I was actually trying to find a regex yesterday that aimed to perform what jsonlite::validate() does. Good to know I don't have to re-invent the whell there. Normalizing the json argument should be done in sc_table - otherwise sc_table_json_post() will cause issues. You should be able to use json strings now with 6b63a60

remotes::install_github("statistikat/STATcubeR@6b63a60")
STATcubeR::sc_table('{"database": "str:database:detouextregsai"}')

@matmo
Copy link
Author

matmo commented Dec 16, 2022

Awesome, nice work!

@matmo matmo closed this Dec 16, 2022
@GregorDeCillia
Copy link
Contributor

You are very welcome.

To clarify: 6b63a60 is part of #32 but my plan is to merge this in the upcoming weeks and create a new tag for v0.6.0. The deprication warning (parameter json_file was renamed to json) will be removed in v1.0.

@matmo
Copy link
Author

matmo commented Feb 20, 2024

Any news on the v0.6 release @GregorDeCillia? Thanks!

@bernhard-da
Copy link
Collaborator

@matmo unfortunately, plans for further development of the package are currently "on-hold" due to time-constraints as @GregorDeCillia has left STAT.

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.

3 participants