-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
[typescript-fetch] add samples of nullable enum #7754
[typescript-fetch] add samples of nullable enum #7754
Conversation
@@ -1119,6 +1119,42 @@ paths: | |||
responses: | |||
200: | |||
description: The instance started successfully | |||
/fake/enum-request: |
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 this file should be copied rather than modified to avoid changing other test data
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.
It's better to make a separate file of minimal yaml?
I'm doing this because I saw this description:
For new test cases, please add to the Fake Petstore 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.
ah, ok, yeah so its fine as it is, 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.
there is still an issue with this test, could you fix it?
https://cloud.drone.io/OpenAPITools/openapi-generator/12016/1/5
a633fb9
to
d35283e
Compare
This comment has been minimized.
This comment has been minimized.
d35283e
to
ce4990e
Compare
@berlysia there are still some samples that need to be updated. |
#7754 (comment) is fixed, but samples have difference. on my env (42 changed, 329 files created)
on CI
|
@berlysia in order to make it easier to review this PR and avoid changing the samples of other generators, I would copy the https://github.com/OpenAPITools/openapi-generator/blob/ce4990ecc3ceba25a62d80e4880f91807a1b35b4/modules/openapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing.yaml file instead of modifying it. @wing328 any objections? |
I will add minimal yaml which contains nullable enum values. It's too much large spec to copy, I think. |
ce4990e
to
1af3421
Compare
schema: | ||
allOf: | ||
- $ref: "#/components/schemas/StringEnum" | ||
type: string | ||
nullable: true |
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.
Oops. OAI/OpenAPI-Specification#1368 (comment)
In JSON Schema, allOf doesn't work like inheritance. It means that the value must conform independently to each of the listed subschemas, and it must also conform to any other conditions specified directly in the schema.
I'll remove this because quoted as above and I've found OAI/OpenAPI-Specification#2244 . Generated codes are roughly usable but this yaml does not conform OpenAPI Spec, currently.
57804e9
to
bdd2318
Compare
@macjohnny Please take another look, this PR just adds samples now. I've finally noticed that "referenced nullable enum" is one of the hottest issue of OpenAPI Generator and OpenAPI Specification.
How we define a nullable variation of defined schema(includes enum)for OAS v3.0 (it's weird along with spec, but useful codes will be generated):
for OAS v3.1(not works currently because it's not implemented):
see. OAI/OpenAPI-Specification#1368 (comment) and OAI/OpenAPI-Specification#1368 (comment) |
bdd2318
to
42e3364
Compare
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
resolves #7745
PR checklist
./bin/generate-samples.sh
to update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example./bin/generate-samples.sh bin/configs/java*
. For Windows users, please run the script in Git BASH.master
@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02)