-
Notifications
You must be signed in to change notification settings - Fork 1.1k
refactor: break down controller-spec #1020
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
Conversation
7997997 to
0a01195
Compare
packages/openapi-v2/src/keys.ts
Outdated
|
|
||
| // Note for review purpose, will be deleted when land the PR: | ||
| // We can have a discussion for each key's name and its value: | ||
| // I will add my proposal in a git comment under this line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The namespace is called ControllerKeys, since they are used for processing controller class metadata.
The name of each key is changed from REST_* to * because the decorators are not tightly coupled with the rest package after being extracted to a single module.
And the values are renamed from rest:* to controller:*
I would like to hear more opinions on the names, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like decoupling REST from the keys here. On top of that though, I think the keys should also have the word spec or something akin to it included as well since the metadata attached to those keys all are related to the specs.
0a01195 to
bdf7c98
Compare
shimks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think organization-wise, controller-spec.ts should be renamed and broken into two or three pieces: operation decorators and others (@api and spec-building functions).
This is because I think the spec-building parts should live separately from the decorators assigning small pieces of spec metadata. Now as for @api I'm not sure whether that should live off separately from either of the other files. Thoughts?
Other than that, as long as TSLint didn't error out at unused imports or whatever, 👍
packages/openapi-v2/src/keys.ts
Outdated
|
|
||
| // Note for review purpose, will be deleted when land the PR: | ||
| // We can have a discussion for each key's name and its value: | ||
| // I will add my proposal in a git comment under this line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like decoupling REST from the keys here. On top of that though, I think the keys should also have the word spec or something akin to it included as well since the metadata attached to those keys all are related to the specs.
Good point! I agree to extract operation decorators to another file, and my personal take is split
So let me summarize what will be in folder
And
|
| export const PARAMETERS_KEY = 'controller:parameters'; | ||
| export const CLASS_KEY = 'controller:class'; | ||
| export const CONTROLLER_SPEC_KEY = 'controller:controller-spec'; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A controller can have metadata for different api styles. It's probably better to use openapi-v2 as the namespace. For example:
openapi-v2:methodsopenapi-v2:parameters
Please note the controller is the target for such metadata, there is no need to add controller to the keys.
Later on, we can have more keys, such as:
openapi-v3:methodsopenapi-v3:parametersgrpc:methodsgrpc:parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah a repeating of controller is too silly and could result in conflicts 🙊 I once named them as swagger:* while finally changed all to controller:* when thinking of we may add decorators of other classes like Response.
I am changing them back, the metadata target is already the best describe for its controller nature.
4df7437 to
cefb7d5
Compare
| ControllerKeys.CLASS_KEY, | ||
| spec, | ||
| ); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in the last commit
4800a92 to
25a72c4
Compare
25a72c4 to
c12cc08
Compare
c12cc08 to
8aec1bd
Compare
Description
related prototype PR: #916
related issue: #753
This PR is the 2nd of the 4 sub PRs I mentioned in #916 (comment)
When upgrade to OpenAPI 3, I break down the functions in
openapi-v3/src/controller-spec.tsinto 6 files, see folder.This PR breaks down
openapi0-v2/src/controller-spec.tsin a similar way, so we can compare the code change when upgrading them to v3 in the PR afterwards.Checklist
npm testpasses on your machinepackages/cliwere updatedpackages/example-*were updated