Unable to override HTTP method from operation template - issue #3636 #3637
Conversation
johanste
requested review from
bterlson,
markcowl,
allenjzhang and
timotheeguerin
as code owners
|
You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/3637/ Check the website changes at https://tspwebsitepr.z22.web.core.windows.net/prs/3637/ |
| @@ -4,7 +4,7 @@ export const $lib = createTypeSpecLibrary({ | |||
| name: "@typespec/http", | |||
| diagnostics: { | |||
| "http-verb-duplicate": { | |||
| severity: "error", | |||
There was a problem hiding this comment.
I don’t feel like I agree with making this a warning, we have the same pattern for other decorators. It doesn’t make sense to specify 2 verb on the exact same node. We should just allow it to override but complain if it’s specified on the same node not operation. Having to suppress that warning when you want to pverrride with op is just feels wrong anyway
There was a problem hiding this comment.
If we want to remove the warning altogether, I would not be opposed to it.
I don't think I'd bother with checking/warn if you specified it twice on the same node, though - I think you get exactly what you asked for.
There was a problem hiding this comment.
So i think the following should be an error
@get @get op op1(): void;
@get @post op op2(): void;
but this should not emit any errors/warning
@get op base(): void;
@post op override is base;
There was a problem hiding this comment.
Hhm I guess duplicate-decorator which is the common diagnostic we emit for duplicate use is a warning not error so that can stay a warning, still it shouldn't need to be suppress and only emit this diagnositc when annotating the same node twice(existing logic is validateDecoratorUniqueOnNode)
There was a problem hiding this comment.
Is getting warnings for multiple decorator instances on the same node here a blocker? I can add another utility function to handle the case where there can only be one instance in a "family" of decorators on a given node, but it feels like a low priority thing. If I wasn't digging around in this code to begin with, I would have said it goes on the backlog... and I'm tempted to say so even if I'm digging around in this code.
There was a problem hiding this comment.
Well from experience seeing how a significant amount of azure spec written don’t see those as warning but errors(warn as error) I do think it is quite a bad experience that will lead to more question we have to keep answering
|
New PR fixing this as design meeting said #3717 |
Fixes #3636