-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: allow custom decorator name for error messages #3625
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
801cbc1 to
9f61b6e
Compare
| return AuthorizeMethodDecoratorFactory.createDecorator( | ||
| AUTHORIZATION_METHOD_KEY, | ||
| spec, | ||
| {decoratorName: '@authorize'}, |
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.
@raymondfeng why do we need provide the decorator name in the options? it could be inferred from the AUTHORIZATION_METHOD_KEY right?
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.
No, the decorator name @authorize cannot be inferred from AUTHORIZATION_METHOD_KEY, which can be a free-form string.
| } | ||
|
|
||
| function testDecorator(spec: object): ClassDecorator { | ||
| return ClassDecoratorFactory.createDecorator('test', 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.
same question here, test and @test seems duplicate, and what would happen if they are different?
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 first argument is the key for the metadata, it can be test or test.key, or other unique strings. The second one is for the decorator function name.
bajtos
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.
👏
| class MyController {} | ||
| }).to.throw( | ||
| /Decorator cannot be applied more than once on class MyController/, | ||
| /ClassDecorator cannot be applied more than once on class MyController/, |
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.
+1 to provide a meaningful default name when no decoratorName metadata was supplied.
jannyHou
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.
👍 LGTM
Extracted and improved from #3617
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈