Skip to content
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

Enum generated FromStr improvements #537

Merged
merged 1 commit into from
May 4, 2024

Conversation

mzeitlin11
Copy link
Collaborator

After using this, I realized a couple issues

  • Writing from_str(s)? reminded me () as the error type for FromStr is terrible since it doesn't impl Error or Display.
  • Enums which fallback to Unknown can have an infallible FromStr.
  • Fallback behavior for from_str and deserialization did not match.

There's another question which doesn't need to be answered here around usage of Unknown as a fallback so that added enum variants are not a breaking change for parsing.

The openapi code currently uses the completely unscientific value of 12 variants for determining when Unknown should be inserted - there's probably an argument for using Unknown on smaller enums than that (or always, but that might start to hurt ergonomics for pretty stable enums).

@arlyon arlyon mentioned this pull request Apr 26, 2024
@arlyon
Copy link
Owner

arlyon commented Apr 30, 2024

There's another question which doesn't need to be answered here around usage of Unknown as a fallback so that added enum variants are not a breaking change for parsing.

I have no strong opinions here. I am tempted to say that if you update your stripe API version you should make sure this library is updated to handle it but I don't think that adding variants is considered a breaking change... I think we could bring the threshhold down a little, perhaps down to 4 or 6, or maybe always insert Unknown for 'regular' enums (as opposed to tagged variants with associated data). I have a feeling most of the churn will come from 'this field has n string values' rather than fundamental differences in response type.

@arlyon
Copy link
Owner

arlyon commented Apr 30, 2024

I will open a ticket on the project and lets move on.

@mzeitlin11 mzeitlin11 merged commit ba6f20d into arlyon:next May 4, 2024
18 checks passed
@mzeitlin11 mzeitlin11 deleted the from_str_tweaks branch May 4, 2024 18:24
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.

2 participants