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.

Sign up for GitHub

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

Jump to bottom

add serializer_rds and serializer_rds3 #387

Merged
merged 12 commits into from
Mar 18, 2019
Merged

add serializer_rds and serializer_rds3 #387

merged 12 commits into from
Mar 18, 2019

Conversation

schloerke
Copy link
Collaborator

Fixes #345

  • change name to serializer_rds
  • add params to serializer fn
  • add new serializer_rds3 with version = "3"
  • in serializer_rds, stop if R version is less than 3.5 as version 3 will fail

@schloerke schloerke requested a review from wch February 22, 2019 21:59
@schloerke
Copy link
Collaborator Author

@shrektan Does this PR work for you?

@codecov-io
Copy link

codecov-io commented Feb 22, 2019 •
edited
Loading

Codecov Report

Merging #387 into master will increase coverage by 1.74%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
+ Coverage   87.67%   89.42%   +1.74%     
==========================================
  Files          27       29       +2     
  Lines        1209     1617     +408     
==========================================
+ Hits         1060     1446     +386     
- Misses        149      171      +22
Impacted Files Coverage Δ
R/serializer-htmlwidget.R 88.88% <ø> (ø) ⬆️
R/serializer-content-type.R 90.9% <ø> (ø) ⬆️
R/serializer-json.R 100% <ø> (ø) ⬆️
R/serializer-html.R 100% <ø> (ø) ⬆️
R/serializer-rds.R 60% <60%> (ø)
R/async.R 97.84% <0%> (ø)
R/plumber-step.R 89.51% <0%> (+4.9%) ⬆️
R/plumber.R 86.31% <0%> (+5.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfd6440...d25e14c. Read the comment docs.

shrektan added a commit to shrektan/apis-plumber that referenced this pull request Mar 1, 2019
@shrektan
doc the new serializer rds and rds3
5e7107f 
ref to rstudio/plumber#387
@shrektan shrektan mentioned this pull request Mar 1, 2019
Closed
@shrektan
Copy link
Contributor

shrektan commented Mar 1, 2019

@schloerke I've tested it. It works great. Thanks!

@schloerke schloerke added this to the v0.5.0 - Next CRAN release milestone Mar 13, 2019
wch
wch approved these changes Mar 13, 2019
View reviewed changes
Copy link
Collaborator

@wch wch left a comment

Choose a reason for hiding this comment

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

After some discussion, we decided that the rds3 serializer should be removed. Otherwise, looks good.

@schloerke schloerke merged commit 0787b8c into master Mar 18, 2019
@schloerke schloerke deleted the shrektan-robject branch March 18, 2019 20:21
schloerke added a commit that referenced this pull request Mar 19, 2019
@schloerke
Merge branch 'master' into byzheng-dynamic
eb00daa 
* master:
  add serializer_rds and serializer_rds3 (#387)
  Support around non-ASCII key values in query string (#396)
  update host from 0.0.0.0 to 127.0.0.1 (and [::] to [::1]) for swagger url only (#376)
  Add support for returning promises from endpoints (#248)
  use httpuv url encode / decode (#355)
  Add support for serializer parameters in plumber block (#356)
@meztez meztez mentioned this pull request Apr 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wch wch wch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@schloerke @codecov-io @shrektan @wch