-
Notifications
You must be signed in to change notification settings - Fork 9.1k
added regex, quoting #638
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
added regex, quoting #638
Conversation
@fehguy It hard to represent RegExp in UI. It very common situation when you present developer with list of possible errors. var regExp = new RegExp('^' + responsesCode.replace(/X/, '\d') + '$'); |
I agree, but that's a tooling concern. The spec, however, I feel is best suited by this flexibility. How else would you say "anything other than a 200" should map here? |
@fehguy very simple, define |
but then it's not simple, it's no longer a "match" statement. It's an "if/else". |
@fehguy RegExp is very tricky to do right. |
@fehguy It's what I would normally expect. Try to write to write regex for not |
@fehguy Moreover during SDK generation you need to generate exception classes with some unique names what string you would use for this purpose? It also impossible to validate if the user supplied two RegExp that overlaps. It's all tooling issues but as I showed above it's UI, code-generation and validation. This is easy to resolve in |
@IvanGoncharov we have regexes in the spec, and there's no escaping that. Pattern Properties are, in fact, part of JSON schema. Do you recommend we get rid of those? |
@fehguy Do you mean If last one, In any case, I understand that there are no other ways to validate arbitrary string except RexExp. Moreover |
Independent of the syntax (regex vs. x), I guess we need some fallback logic here allowing an exact match to override an regex match, allowing The "everything else than 200" otherwise becomes e.g. |
@ePaul with
You forget anchors so it should be |
Because of the way that RFC 7231 defines how status codes should be handled I would be in favor of supporting 2XX, 3XX, 4XX, 5XX as explicit values in addition to regular 3 digit values. I don't see any practical value to patterns like 20X or more flexible regex patterns as there is no logical grouping of status codes beyond the first digit. |
@IvanGoncharov as we are matching only HTTP status codes (i.e. 3-digit strings), no anchoring is necessary. (Yes, my regex matches many other non-status code strings too, but that doesn't matter.) |
@darrelmiller Your solution is solving the absolute majority of use cases and also super simple to implement. And it's very easy to validate using JSON Schema, so no problems with confused users. |
Agreeing with @darrelmiller and the rest. Seems simpler, and actually covers the use case in the original issue. I don't really see anyone wanted to capture a different range, as the status codes don't have a logical grouping, afaik, other than the hundred-level grouping. What we may want to consider is allowing for grouping in the key with comma-separated values, so |
Updated to replace regex with a
|
@OAI/tdc please approve or reject |
👍 I'd suggest we drop |
👍 but allow |
I would even go as far as allowing only the lowercase version ( |
I think having aliases is bad practice and should be used only for backward compatibility. I also think this feature should be limited to practical set.
So |
Just to make sure ... there is no point in documenting an 1xx response, right? These are only intermediate responses on protocol level, which will be followed by a "real" one, or protocol switching stuff (not HTTP anymore). (So the fixed set proposed by @IvanGoncharov, in addition to the numeric ones, would be enough.) |
@ePaul In theory it might be useful to document the 100 status code. Knowing if a server supports the Expect: 100 Continue dance and what is the maximum payload size allowed could be useful. Regarding the letter casing, as I understand it, most of the OpenAPI properties are case sensitive, so it would seem prudent to keep this convention. Also, most of OpenAPI tends towards lower case letters and RFC 7231 uses 1xx, 2xx,3xx, etc so that would seem like the obvious choice. |
@fehguy Some potential ambiguity:
|
thanks for the feedback--i suggest that we go with If there are strong objections to this, let's reopen the issue again. |
👍 |
@fehguy What you've just merged is not actually what you've proposed in the last comment. |
@ePaul i'm saying that's the rule. We are not supporting regex. I've just explained the idea in a regex. And as I said, we can reopen if we have an issue. So:
|
@fehguy Any chance you would consider this regex |
The commit uses uppercase
This language also allows
There is also the suggestion to allow comma-separated list, such as |
I will fix the upper-case, i did in the sample regex but not the doc. I don't agree with the comma-separated list so I didn't add that. Will make sure docs are updated. |
this needs to be updated to lower case, skipping the change for |
added regex, quoting [Manually poted merge] Co-authored-by: Tony Tam <[email protected]>
Fixes #516