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

Consolidate swagger type information #388

Merged
merged 14 commits into from Mar 21, 2019
Merged

Consolidate swagger type information #388

merged 14 commits into from Mar 21, 2019

Conversation

schloerke
Copy link
Collaborator

Fixes #351

This consolidates all swagger type information into one location. If a new type is supported in the future, only one spot will need to be added.

@schloerke
Copy link
Collaborator Author

Old:

switch(type,
"bool" = ,	
"logical" = "boolean",

"double" = ,
"numeric" = "number",

"int" = "integer",

"character" = "string"
)

New:

addSwaggerInfo(
  "boolean",
  c("bool", "boolean", "logical"),
  "[01tfTF]|true|false|TRUE|FALSE",
  as.logical
)
addSwaggerInfo(
  "number",
  c("dbl", "double", "float", "number", "numeric"),
  "-?\\\\d*\\\\.?\\\\d+",
  as.numeric
)
addSwaggerInfo(
  "integer",
  c("int", "integer"),
  "-?\\\\d+",
  as.integer
)
addSwaggerInfo(
  "string",
  c("chr", "str", "character", "string"),
  "[^/]+",
  as.character
)

@schloerke
Copy link
Collaborator Author

I am worried about this change. https://github.com/trestletech/plumber/pull/388/files#diff-c6fcef256c9135dbef4ee90414a8074aR54

Should the regex be updated to only look for known types? Or should the regex look for a pattern of <VAR:TYPE> and complain if TYPE is unknown.

@schloerke schloerke requested a review from wch February 22, 2019 22:11
@codecov-io
Copy link

codecov-io commented Feb 22, 2019

Codecov Report

Merging #388 into master will increase coverage by 0.03%.
The diff coverage is 98.03%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #388      +/-   ##
=========================================
+ Coverage   89.26%   89.3%   +0.03%     
=========================================
  Files          29      29              
  Lines        1435    1449      +14     
=========================================
+ Hits         1281    1294      +13     
- Misses        154     155       +1
Impacted Files Coverage Δ
R/plumber-step.R 88.37% <100%> (ø) ⬆️
R/query-string.R 91.13% <100%> (+0.99%) ⬆️
R/swagger.R 98.55% <92.3%> (-1.45%) ⬇️

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 0787b8c...12279c6. Read the comment docs.

@schloerke schloerke added this to the v0.5.0 - Next CRAN release milestone Mar 13, 2019
R/swagger.R Outdated Show resolved Hide resolved
R/swagger.R Outdated Show resolved Hide resolved
@schloerke schloerke requested a review from wch March 14, 2019 15:28
* 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)
@schloerke
Copy link
Collaborator Author

This currently will cause an error if a type isn't found.

> tmp <- tempfile("tmp")
> cat(file = tmp, "
+ #* @get /barret/<barret:int>
+ function(req, res) {
+   'barret-int!'
+ }
+ #* @get /barret/<barret:asdf>
+ function(req, res) {
+   'barret-asdf!'
+ }
+ ")
> plumb(tmp)$run(port = 1234)
Error in plumberToSwaggerType(types) : Unrecognized type: asdf

Before, I would at least be able to find the /barret/<barret:int> route. The /barret/<barret:asdf> route was not reachable.

wch
wch approved these changes Mar 19, 2019
@schloerke schloerke merged commit 2636966 into master Mar 21, 2019
@schloerke schloerke deleted the byzheng-dynamic branch March 21, 2019 17:50
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.

None yet

4 participants