Skip to content

feat: add literal_enums setting to produce Literal[...] instead of enum classes. #1114

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

Closed
wants to merge 16 commits into from

Conversation

emosenkis
Copy link
Contributor

While attempting to generate a client for https://api.railz.ai/swagger/v2, I ran into duplicate enum values. I tried using the fixes from #725 and #1095 but neither was sufficient because while some duplicates result from case insensitivity, others result from characters (e.g. +) that are stripped when generating the Python name for the value. Allowing enums to be implemented as Literals provides a good alternative that should result in more readable code when an API defines enums that really are a collection of possible values not amenable to representation as a Python enum.

This solution works for me and if you're amenable to the idea, I can add tests, etc. to get it ready for merging. Down the line, I can imagine allowing more configuration options, such as using literal enums only for int enums, only for enums with duplicate values, or only for enums that match a regex but I don't know if you're interested in allowing that sort of granular configuration of generated types.

@emosenkis emosenkis changed the title Add literal_enums option. feat: add option to produce Literal[...] instead of enum classes. Sep 2, 2024
@dbanty
Copy link
Collaborator

dbanty commented Sep 2, 2024

This looks great, thanks!

Also related is #1076, though that one didn't provide a config option and didn't use the literals in the endpoint code, just added it as an additional generated piece.

I think I like this approach better—where the style is selected and only one or the other is generated, rather than both. I guess we'll need another new snapshot test for this style—probably a brand new OpenAPI document rather than keeping another variant of the massive end_to_end_tests/golden-record around.

I do think the more granular controls could also be good in the future. Someone might like to generate most of their enums as enum classes, but then opt in to Literal for a few that can't map well.

@chrisguillory
Copy link

This PR also unblocked our generation of:

 pipx run openapi-python-client generate --url https://fivetran.com/assets-docs/openapi/file_v1.json

which errors with:

ValueError: Duplicate key RE_SYNC in Enum

Thanks!

@emosenkis emosenkis changed the title feat: add option to produce Literal[...] instead of enum classes. feat: add literal_enums setting to produce Literal[...] instead of enum classes. Sep 5, 2024
@emosenkis
Copy link
Contributor Author

@dbanty I think this should be ready to merge so long as you're happy with how I implemented it. Fixing the various type errors in the generated code uncovered quite a few bugs in my code and a possibility of non-deterministic output due to ruff making different choices depending on the ordering of imports when one of them is unused. I added a parse-time check that the value is in the enum so that it will be equally as strict as regular enums.

@emosenkis
Copy link
Contributor Author

I merged in main and have pdm check passing for me locally.

@emosenkis
Copy link
Contributor Author

Ok, now I've actually tested with Python 3.8 and fixed the last few usages typing features that are only available from Python 3.10. Everything should pass this time.

@eli-bl
Copy link
Collaborator

eli-bl commented Sep 10, 2024

I know I'm commenting a bit late, but... I'm trying to understand the decision to make this a whole new class, instead of adding an attribute to EnumProperty. I feel like adding a LiteralEnumProperty class both increases the amount of new logic required in other files to handle the new class— even in cases when the behavior isn't actually different from EnumProperty (for instance in merge_properties.py)— and also makes it a bit harder to see what the significant implementation differences are between those two classes (which I think may be relatively small compared to the amount of duplicated boilerplate code, although I haven't compared them line by line). Was there a particular reason for going this way?

@emosenkis
Copy link
Contributor Author

@eli-bl I didn't think very hard about this at the time because I wasn't yet thinking about contributing it upstream. If I remember correctly, the main reason I didn't use EnumProperty class is that the values attribute needs to be a dict for EnumProperty and a set for LiteralEnumProperty. It's also true that the two are mutually incompatible so reusing the same class would result in if branches in virtually every template macro and many of the Python functions.

At the end of the day, I've spent about as much time as I can justify on getting this upstreamed. If someone wants to take it in a different direction I won't be offended but I don't have time to make a big change like unifying the two classes. Either way, I would really like for this feature to be accepted in some form!

@emosenkis
Copy link
Contributor Author

@dbanty Test coverage is now 100% except the StrEnum / str, Enum version-gated definitions.

@dbanty dbanty mentioned this pull request Oct 20, 2024
dbanty added a commit that referenced this pull request Oct 20, 2024
This is just #1114 but with some docs & changeset

---------

Co-authored-by: Eitan Mosenkis <[email protected]>
Co-authored-by: Eitan Mosenkis <[email protected]>
Co-authored-by: Dylan Anthony <[email protected]>
@knope-bot knope-bot bot mentioned this pull request Oct 20, 2024
@dbanty
Copy link
Collaborator

dbanty commented Oct 20, 2024

Thank you so much for this! I had to update a snapshot and I added a mention to the readme both over in #1142—which is merged! This fixes an issue that's clearly bumped into a lot given the number of attempts at solving it 😅.

@dbanty dbanty closed this Oct 20, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 20, 2024
> [!IMPORTANT]
> Merging this pull request will create this release

## Features

- update Ruff to >=0.2,<0.8 (#1137)
- Add UUID string format. Thanks @estyrke! (#1140)
- Support OpenAPI 3.1 prefixItems property for arrays. Thanks @estyrke!
(#1141)

### Add `literal_enums` config setting

Instead of the default `Enum` classes for enums, you can now generate
`Literal` sets wherever `enum` appears in the OpenAPI spec by setting
`literal_enums: true` in your config file.

```yaml
literal_enums: true
```

Thanks to @emosenkis for PR #1114 closes #587, #725, #1076, and probably
many more.
Thanks also to @eli-bl, @expobrain, @theorm, @chrisguillory, and anyone
else who helped getting to this design!

## Fixes

- Typo in docstring (#1128)

### Use literal value instead of `HTTPStatus` enum when checking
response statuses

Python 3.13 renamed some of the `HTTPStatus` enum members, which means
clients generated with Python 3.13 may not work
with older versions of Python. This change stops using the `HTTPStatus`
enum directly when checking response statuses.

Statuses will still be checked for validity at generation time, and
transformed into `HTTPStatus` _after_ being checked
at runtime.

This may cause some linters to complain.

Co-authored-by: knope-bot[bot] <152252888+knope-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants