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

Add tags to swagger specification #230

Merged
merged 17 commits into from
Jan 23, 2018
Merged

Conversation

svdwoude
Copy link
Contributor

Adding functionality to parse tags from plumber annotation and add to swagger-json.

Example:

#* @apiTitle Tags Example
#* @apiTag name1 description1
#* @apiTag name2 description2
#* @apiTag name3_with_whitespaces description2


#* Example endpoint with one tag
#* @tag name1
#* @get /f1
function() {

}

#* Example endpoint with multiple tags
#* @tag name1
#* @tag name2
#* @get /f2
function() {}

#* Example endpoint with unmatched tags
#* @tag randomName
#* @get /f3
function() {}

#* Example endpoint with tag with whitespaces
#* @tag name3_with_whitespaces
#* @get /f4
function() {}

Which would create a swagger-ui with 4 tags:

  • name1

    • /f1
    • /f2
  • name2

    • /f2
  • randomName

    • /f3
  • name3 with whitepsace

    • /f4

Screenshot below:
plumber-tags-exmaple

@CLAassistant
Copy link

CLAassistant commented Jan 21, 2018

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Jan 21, 2018

Codecov Report

Merging #230 into master will increase coverage by <.01%.
The diff coverage is 86.2%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   88.18%   88.18%   +<.01%     
==========================================
  Files          24       24              
  Lines        1193     1219      +26     
==========================================
+ Hits         1052     1075      +23     
- Misses        141      144       +3
Impacted Files Coverage Δ
R/swagger.R 71.92% <0%> (-1.29%) ⬇️
R/parse-globals.R 100% <100%> (ø) ⬆️
R/plumber-step.R 82.14% <66.66%> (-0.58%) ⬇️
R/parse-block.R 96.48% <93.33%> (-0.3%) ⬇️

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 65af6a9...1864670. Read the comment docs.

Copy link
Contributor

@trestletech trestletech left a comment

Choose a reason for hiding this comment

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

Looking good! Just a couple of things to revisit.

R/parse-block.R Outdated
@@ -179,6 +180,18 @@ parseBlock <- function(lineNum, file){
params[[name]] <- list(desc=paramMat[1,5], type=type, required=reqd)
}

tagMat <- stringi::stri_match(line, regex="^#['\\*]\\s*@tag\\s+(\\S.+)\\s*")
if (!is.na(tagMat[1,1])){
t <- gsub("_"," ",stri_trim_both(tagMat[1,2]))
Copy link
Contributor

Choose a reason for hiding this comment

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

The underscore-to-space translation feels like a little too much magic to me. I think I'd prefer to just not handle whitespace for now and if people start asking for it then deal with it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. Updated in 8beafad

},
apiTag={
tagMat <- stringi::stri_match(def, regex="^\\s*(\\w+)\\s+(\\S.+)\\s*$")
name <- gsub("_"," ",tagMat[1,2])
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. Updated in 8beafad

R/plumber-step.R Outdated
@@ -133,6 +134,9 @@ PlumberEndpoint <- R6Class(
if (!missing(responses)){
self$responses <- responses
}
if(!missing(tags) && !is.null(tags)){
self$tags <- I(tags)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand the I() call here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we only have one tag, we want to make sure it's boxed in.
In JSON we want tags: ["tagName"] instead of tags: "tagName"

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great to have a comment inline for that so I don't lose track of it in the future, but that makes sense.

@@ -28,7 +28,8 @@ test_that("parseGlobals works", {
"#' @apiBasePath basepath",
"#' @apiSchemes schemes",
"#' @apiConsumes consumes",
"#' @apiProduces produces")
"#' @apiProduces produces",
"#' @apiTag tag description")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add multiple tag definitions here for this test? And perhaps also a test that has redundant apiTag definitions? I'd think that should fail but I don't believe that it does today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multiple tags added in test.
I've also added a test to check for redundant apiTag definitions (= duplicates). Since swagger is pretty relaxed about unused tag definitions, I did not add a test to see wether all apiTag definitions are also referenced in at least one endpoint.

tagMat <- stringi::stri_match(def, regex="^\\s*(\\w+)\\s+(\\S.+)\\s*$")
name <- gsub("_"," ",tagMat[1,2])
description <- tagMat[1,3]
fields$tags <- rbind(fields$tags,data.frame(name=name,description=description))
Copy link
Contributor

Choose a reason for hiding this comment

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

should we do a stringsAsFactors = FALSE here? I usually prefer it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Added in 1f509a0

@jeffkeller87
Copy link

Really excited to see more code decoration support!

@trestletech
Copy link
Contributor

I saw the commits come in; let me know when this is ready for rereview and I'll take another pass!

@svdwoude
Copy link
Contributor Author

Ready for review!

@trestletech
Copy link
Contributor

Looks great! I pushed one more commit for some doc updates and to add you to the DESCRIPTION file. I pulled the email from your public GitHub profile, but let me know if you'd prefer to be removed or use a different address.

If that looks good, I'll go ahead and pull it in once tests pass!

@svdwoude
Copy link
Contributor Author

svdwoude commented Jan 23, 2018

Sorry about squeezing in that last typo fix.
Probably better to use the email I did my commits with.
Changed that meanwhile on my profile.

Thanks for checking with me ☺️

@trestletech trestletech merged commit f98b41b into rstudio:master Jan 23, 2018
@trestletech
Copy link
Contributor

🎉

Thanks again for you contribution!

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.

5 participants