Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

improve UnionProperty behavior for anyOf/oneOf, lists of types, and nullable enums#1121

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
eli-bl wants to merge7 commits intoopenapi-generators:main
base:main
Choose a base branch
Loading
fromeli-bl:issue1120-nullable-enum

Conversation

eli-bl
Copy link
Collaborator

@eli-bleli-bl commentedSep 13, 2024
edited
Loading

Fixes#1120.

There are two related changes here:

First, when processing the anyOf/oneOf items in a union, check whether each item will generate a named Python class (i.e. is it an enum or a model). Ifexactly one of them will, then we shouldnot use a synthetic name likexyz_type_1; instead just use the original namexyz. (If more than one of them will, then we do need to add name suffixes; if none of them will, then it doesn't really matter since none of the synthetic names will show up in Python code anyway).

That fixes the case described in the issue where nullable object/enum classes got an unnecessary "Type1" suffix. It's a breaking change, but, I think, a desirable one— I don't think anyone prefers to have "Type1" added in those cases, and depending on that would be a bad idea anyway since it's an obscure implementation detail. So I haven't gated it behind a config option.

Second, if we're creating a union andtype has been explicitly specified— these were the cases where a bunch of spurious -Type1, -Type2Type1, etc. were being created, because of faulty logic that assumed every explicit type had to be added separately to the union. I believe the correct behavior is:

  • If the union is just because there are multipletypes, then go ahead and add a Property for each one.
  • If there is atype ortypes, but there isalsoanyOf oroneOf, then we don't need to add anything, because the schemas in theanyOf/oneOf list already describe what kind of values there can be. (We could add a check to make sure the types aren't contradictory— for instance, iftype is["string", "number"] but theanyOf variants are objects— but that's out of scope here.)

The second issue was also the reason for the weird behavior ofnullable with enums in 3.0, because we are handlingnullable by converting the type into a union.

tokoro10g reacted with thumbs up emoji
@eli-bleli-bl marked this pull request as ready for reviewSeptember 13, 2024 01:44
("3_0_implicit_type", {"nullable": True}),
("3_0_explicit_type", {"type": "string", "nullable": True}),
],
)
def test_property_from_data_str_enum_with_null(
Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The reason that this unit test wasn't catching the problem was that there several ways the same nullable enum could be declared, and it was only testing one of them.

@eli-bleli-bl marked this pull request as draftSeptember 13, 2024 22:39
@eli-bleli-bl marked this pull request as draftSeptember 13, 2024 22:39
@eli-bleli-bl changed the titlefix class generation for nullable enumsimprove UnionProperty behavior for anyOf/oneOf, lists of types, and nullable enumsSep 14, 2024
@eli-bleli-blforce-pushed theissue1120-nullable-enum branch 3 times, most recently from1c0d1ec tob6f22b9CompareSeptember 16, 2024 17:48
@eli-bleli-blforce-pushed theissue1120-nullable-enum branch fromb6f22b9 to500f363CompareSeptember 16, 2024 17:51
@eli-bleli-bl marked this pull request as ready for reviewSeptember 16, 2024 17:54
@eli-bleli-bl requested a review fromdbantySeptember 16, 2024 17:54
@dbantydbanty added 🐞bugSomething isn't working 🥚breakingThis change breaks compatibility labelsOct 20, 2024
@eli-bleli-blforce-pushed theissue1120-nullable-enum branch 3 times, most recently from4f416e2 to8696277CompareDecember 7, 2024 00:54
@eli-bleli-blforce-pushed theissue1120-nullable-enum branch from8696277 tobfb0df1CompareDecember 7, 2024 00:58
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@dbantydbantyAwaiting requested review from dbanty

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
🥚breakingThis change breaks compatibility🐞bugSomething isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

union types and nullables can create unnecessary class suffixes (and related problems)
2 participants
@eli-bl@dbanty

[8]ページ先頭

©2009-2025 Movatter.jp