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

feat: Create @loopback/openapi-v2 #804

Merged
merged 5 commits into from
Jan 3, 2018
Merged

feat: Create @loopback/openapi-v2 #804

merged 5 commits into from
Jan 3, 2018

Conversation

jannyHou
Copy link
Contributor

@jannyHou jannyHou commented Dec 15, 2017

connect to #753
Replace #800

  • create a new package openapi-specgen
  • move route decorators and related test cases from rest to openapi-specgen

I will put more release details after the code looks good.

@jannyHou jannyHou changed the title feat: Create @loopback/openapi-specgen feat: Create @loopback/openapi-specgen[WIP] Dec 18, 2017
@jannyHou jannyHou force-pushed the openapi-specgen branch 2 times, most recently from fca4e22 to c4b8c94 Compare December 27, 2017 22:18
@jannyHou jannyHou self-assigned this Dec 28, 2017
@jannyHou jannyHou changed the title feat: Create @loopback/openapi-specgen[WIP] feat: Create @loopback/openapi-specgen Jan 1, 2018
@bajtos
Copy link
Member

bajtos commented Jan 2, 2018

Since this is mostly moving files around, there is little to review here. The changes look reasonable after a quick review.

My only objection is against the package name @loopback/openapi-specgen - the code here is not generating the spec, it's providing decorators to define the specification. Can we come up with a better name, one that conveys this intent in a better way? Few examples that come to my mind: @loopback/openapi-metadata, @loopback/openapi-decorators. Or perhaps simply @loopback/openapi, meaning "the glue between loopback and openapi-spec".

@jannyHou
Copy link
Contributor Author

jannyHou commented Jan 2, 2018

@bajtos Thanks for review, this package will contain all of the openapi spec generation code, like function getControllerSpec, not only the route decorators.

Functions like getModelMetadata in PR #813 will be moved to this repo as well.

Copy link
Contributor

@shimks shimks left a comment

Choose a reason for hiding this comment

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

LGTM

@raymondfeng
Copy link
Contributor

@jannyHou The module is the home for openapi related metadata, including decorators for describing the metadata and utility methods for inspecting the metadata. It makes sense to me to name it as @loopback/openapi or @loopback/openapi-metadata.

@jannyHou
Copy link
Contributor Author

jannyHou commented Jan 2, 2018

including decorators for describing the metadata and utility methods for inspecting the metadata

That's true, specgen doesn't imply it contains decorators for describing the metadata.

And I prefer @loopback/openapi

@raymondfeng I had a talk with @kjdelisle, we think at this moment @loopback/openapi-v2 would be a good name for it, it specifies the version, and we can rename it to @loopback/openapi-v3 after upgrading.


This package contains:

- Decorators that describe LoopBack artifacts as metadata.
Copy link
Contributor

Choose a reason for hiding this comment

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

Decorators that apply REST api mapping metadata to controller classes and their members.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raymondfeng as I have mentioned in #804 (comment), my intension to have this package is to organize all the openapi related decorators and spec generation functions together. Not only for controller classes....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raymondfeng How about "Decorators that describe LoopBack artifacts as openapi metadata."? I missed an important word "openapi" in this commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

These decorators are created for application developers to describe REST mapping of controller classes, right?

This package contains:

- Decorators that describe LoopBack artifacts as metadata.
- Uilities that tranfer LoopBack remoting metadata to swagger specifications.
Copy link
Contributor

Choose a reason for hiding this comment

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

Utilities that inspect controller classes to build OpenAPI v2 (Swagger) specifications from REST api mapping metadata.

@raymondfeng raymondfeng changed the title feat: Create @loopback/openapi-specgen feat: Create @loopback/openapi-v2 Jan 2, 2018
## Overview

The package has functions described above for LoopBack controller classes.
Decorators apply REST api mapping metadata to controller classes and their members. And utilities that inspect controller classes to build OpenAPI v2 (Swagger) specifications from REST api mapping metadata.
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate of the This package contains section?

Copy link
Contributor Author

@jannyHou jannyHou Jan 2, 2018

Choose a reason for hiding this comment

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

@raymondfeng There is a difference: the top level description is for ALL artifacts :) And the Overview section says currently what the package has(functions for controller classes). And we will add more for other artifacts if need.

Copy link
Contributor

@kjdelisle kjdelisle left a comment

Choose a reason for hiding this comment

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

Almost there! Just need a couple quick fixes:

  • Rename the openapi-spec package to openapi-v2-spec to make it clear which version of the OpenAPI specification this package is meant to represent.
  • Add in the missing newlines
  • Reword the commit message to say feat: Create @loopback/openapi-v2-spec
  • Bump the minimum engine to 8.

@@ -0,0 +1,3 @@
*.tgz
dist*
package
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline

@@ -0,0 +1 @@
package-lock=false
Copy link
Contributor

Choose a reason for hiding this comment

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

newline

"version": "1.0.0-alpha1",
"description": "Processes openapi v2 related metadata",
"engines": {
"node": ">=6"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please set this to >=8, as we've decided not to support Node 6 for our release.

},
"include": ["src", "test"]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

newline

This package contains:

- Decorators that describe LoopBack artifacts as OpenAPI v2 (Swagger) metadata.
- Utilities that tranfer LoopBack metadata to OpenAPI v2 (Swagger) swagger specifications.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: transfer

@jannyHou
Copy link
Contributor Author

jannyHou commented Jan 3, 2018

Rename the openapi-spec package to openapi-v2-spec

This is out of the scope of this PR and I don't think it's a quick fix. A simple replacement affects 50 files and we also need to check example repos.

I will address it in the next PR(upgrading from v2-->v3)

@jannyHou jannyHou force-pushed the openapi-specgen branch 2 times, most recently from 754044e to 2faaae5 Compare January 3, 2018 16:08
@kjdelisle
Copy link
Contributor

kjdelisle commented Jan 3, 2018

This is out of the scope of this PR and I don't think it's a quick fix. A simple replacement affects 50 files and we also need to check example repos.

Agreed, as long as the newline fixes are put into place, LGTM.

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

:shipit:

"api-docs",
"src"
],
"repository": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the following so that the package will be published with public access with npm run release.

"publishConfig": {
    "access": "public"
  },

@raymondfeng
Copy link
Contributor

Do you want to change the commit message to be feat: create @loopback/openapi-v2?

@jannyHou
Copy link
Contributor Author

jannyHou commented Jan 3, 2018

@raymondfeng 😀 my bad, forgot to change the commit message.

@jannyHou jannyHou merged commit 4ddd4bc into master Jan 3, 2018
@jannyHou jannyHou removed the review label Jan 3, 2018
@jannyHou jannyHou deleted the openapi-specgen branch January 3, 2018 20:23
@@ -25,6 +25,7 @@
"dependencies": {
"@loopback/context": "^4.0.0-alpha.24",
"@loopback/core": "^4.0.0-alpha.26",
"@loopback/openapi-v2": "1.0.0-alpha1",
Copy link
Member

Choose a reason for hiding this comment

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

@jannyHou The version number should have been 1.0.0-alpha.1 (notice the dot between "alpha" and "1"), this is the scheme we are using in other packages (look at lines 26 and 27 above for examples).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Raymond already created a PR to fix the version.

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.

6 participants