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 serializers. Add serializer_headers() and as_attachment() #585

Merged
merged 19 commits into from
Jul 21, 2020

Conversation

schloerke
Copy link
Collaborator

@schloerke schloerke commented Jul 3, 2020

Fixes: #455
Fixes: #482

  • Build each serializer on top of one another to avoid copy/pasta
  • Add serializer_headers(headers)
  • Have all text-like content return charset=UTF-8
  • Use an .onLoad() method for adding the serializers. (Allows for globals to not be directly altered while the package is loading.)
  • Remove all serializer files and merge into serializer.R
  • Added force argument to allow serializers to be overwritten

PR task list:

  • Update NEWS
  • Add tests
  • Update documentation with devtools::document()
  • Add @ycphs as author to NEWS

* Build each serializer on top of one another to avoid copy/pasta
* Add serializer_headers(headers)
* Have all text-like content return `charset=UTF-8`
* Use an `.onLoad()` method for adding the serializers. (Allows for globals to not be directly altered while the package is loading.)
* Remove all serializer files and merge into serializer.R
@schloerke schloerke added this to the v0.5.0 - Next CRAN release milestone Jul 3, 2020
@schloerke schloerke self-assigned this Jul 3, 2020
@schloerke schloerke changed the title Restructure serializers. Add serializer_headers Restructure serializers. Add serializer_headers(). Add as_attachment() Jul 3, 2020
@schloerke schloerke requested a review from cpsievert July 6, 2020 13:59
R/serializer.R Outdated Show resolved Hide resolved
R/serializer.R Outdated Show resolved Hide resolved
R/serializer.R Outdated Show resolved Hide resolved
R/serializer.R Outdated Show resolved Hide resolved
@cpsievert cpsievert self-requested a review July 9, 2020 20:40
Copy link
Contributor

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

See comments about serializer_text()

blairj09 added a commit to blairj09/plumber that referenced this pull request Jul 13, 2020
blairj09 added a commit to blairj09/plumber that referenced this pull request Jul 16, 2020
schloerke and others added 4 commits July 20, 2020 10:27
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
* Replace `jsonlite::fromJSON()` with `jsonlite::parseJSON(simplifyVector = TRUE)`
* Add `serializer_print()`, `serializer_format()`, and `serializer_cat()`

* Fixed braces in readme
@schloerke schloerke requested a review from cpsievert July 20, 2020 17:01
Copy link
Contributor

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

🎉 🌮 🎉

@schloerke schloerke changed the title Restructure serializers. Add serializer_headers(). Add as_attachment() Restructure serializers. Add serializer_headers() and as_attachment() Jul 21, 2020
@schloerke schloerke merged commit ea9cdeb into master Jul 21, 2020
@schloerke schloerke deleted the content_type_and_disposition branch July 21, 2020 21:01
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.

add force = FALSE param to addSerializer function
4 participants