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

Restructure body parsers to use content type. Add many parsers #584

Merged
merged 79 commits into from Jul 31, 2020

Conversation

schloerke
Copy link
Collaborator

@schloerke schloerke commented Jul 3, 2020

Fixes #583
Fixes #586

Reference: https://community.rstudio.com/t/uploading-tsv-files-to-plumber-api/71705/3

Changes:

  • Restructure parser list to use the pattern as the key and parser as the value.
    • Allows the same parser to exist under different content type values.
    • Provide duplicate parsers for commonly seen content types
  • Allow for arguments to be supplied to parsers
  • Add tsv parser
  • Add csv parser
  • If a content-type of application/octet is provided, inspect the file name to see what it's mime-type is and use that as the content type value.
    • Allows for RDS files to be shipped using application/octet, but processed using application/rds which maps to parser_rds()
  • Use other parsers to reduce copy/pasta
  • Move the JSON first inspection after the content type has a chance to be used. Avoids possible situations where the plain text starts with a {, but is truly plain text and not JSON.
  • Rename (new for next release) functions to snake case. (add_parser(), parse_body(), ...)

PR task list:

  • [NA] Update NEWS
  • Add tests
  • Update documentation with devtools::document()

@meztez Can you take a look at this PR.

meztez and others added 12 commits Jul 3, 2020
* Restructure parser list to use the pattern as the key and parser as the value. Allows the same parser to exist under different content type values.
* Provide duplicate parsers for commonly seen content types
* add tsv parser
* add csv parser
* Allow for arguments to be supplied to parsers
* use other parsers to reduce copy/pasta
@schloerke schloerke added this to the v0.5.0 - Next CRAN release milestone Jul 3, 2020
@schloerke schloerke self-assigned this Jul 3, 2020
R/parse-body.R Outdated Show resolved Hide resolved
@meztez
Copy link
Collaborator

meztez commented Jul 3, 2020

This is masterfully crafted code. Beautiful. Loves the generalization where parser reuse other parser. Last time I've seen that great of a collab was Snoop & Dre.

Seriously. Great stuff.

@schloerke
Copy link
Collaborator Author

schloerke commented Jul 3, 2020

😆😆 Made my day!

R/parse-body.R Outdated Show resolved Hide resolved
R/parse-body.R Outdated Show resolved Hide resolved
@schloerke schloerke requested a review from cpsievert Jul 6, 2020
R/parse-body.R Outdated Show resolved Hide resolved
R/json.R Outdated Show resolved Hide resolved
R/plumb-block.R Outdated Show resolved Hide resolved
@schloerke schloerke mentioned this pull request Jul 28, 2020
2 tasks
schloerke and others added 2 commits Jul 28, 2020
* master:
  Replace plus sign with space character when parsing a query string (#618)
Co-authored-by: Bruno Tremblay <bruno.tremblay@lacapitale.com>
R/parse-body.R Outdated Show resolved Hide resolved
R/parse-body.R Outdated Show resolved Hide resolved
R/parse-body.R Show resolved Hide resolved
R/plumber-step.R Outdated Show resolved Hide resolved
R/parse-body.R Outdated Show resolved Hide resolved
Copy link
Contributor

@cpsievert cpsievert left a comment

LGTM pending last few comments

@schloerke schloerke changed the title Restructure body parsers to use content type. Add TSV and CSV parsers Restructure body parsers to use content type. Add many parsers Jul 31, 2020
@schloerke schloerke merged commit 0254d8b into master Jul 31, 2020
15 of 16 checks passed
@schloerke schloerke deleted the post_body_content_type branch Jul 31, 2020
@meztez
Copy link
Collaborator

meztez commented Jul 31, 2020

Pop a can of Dr Pepper, you did it!!!!

@schloerke
Copy link
Collaborator Author

schloerke commented Jul 31, 2020

😌 Hope to get #598 this afternoon so we can test the two of them together.

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