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

feat(next): keep original string on unknown variants #612

Merged
merged 1 commit into from
Nov 2, 2024

Conversation

lasantosr
Copy link

Summary

This PR aims to keep the original values for unknown variants, if they are generated.

This allows downstream crates to act on them, for example logging the unexpected value. This is useful for example on the ApiVersion for the webhook versioning, in order to be able to properly determine if the event comes from a newer api version.

Checklist

@lasantosr lasantosr force-pushed the feature/keep-unknown-variants branch from d808a99 to b0eb447 Compare September 17, 2024 18:11
@arlyon
Copy link
Owner

arlyon commented Sep 24, 2024

Thanks for doing this. I am for it but I will let @mzeitlin11 pull the trigger

@mzeitlin11
Copy link
Collaborator

Thanks @lasantosr! Main initial question is both the purpose of the From<String> impl and its implementation. There should already be a FromStr impl for each enum - best if the raw string to enum variant logic doesn't get duplicated (since some of these enums are so giant, the FromStr impls are already the heaviest functions according to cargo bloat).

@lasantosr
Copy link
Author

The purpose of From<String> is to have the infallible version taking directly the owned String instead of duplicating it.

I thought about replacing the implementation, removing FromStr instead of duplicating for enums with the unknown variant, which can't fail.

I didn't consider timing, so I can update the PR with one of the following, or any other you might consider (for enums with unknown variant only):

  • removing From<String> impl
  • removing FromStr impl
  • updating FromStr impl to call From<String>
  • updating From<String> impl to call FromStr

@arlyon
Copy link
Owner

arlyon commented Sep 25, 2024

I think it is reasonable to clone in the exceptional case of encountering an unknown string. When calling stripe apis the bottleneck will be the network (meaning the additional resource cost of a clone is negligible). So my vote is option 1.

@mzeitlin11
Copy link
Collaborator

I also like 1 for the reasons given above too. The FromStr impl for these types also already uses std::convert::Infallible for the error, so the inability to fail is already represented

@arlyon
Copy link
Owner

arlyon commented Sep 26, 2024

@lasantosr ping me here when you have removed the impl and I will get it released :)

@lasantosr lasantosr force-pushed the feature/keep-unknown-variants branch from b0eb447 to d4583c2 Compare September 29, 2024 16:34
@lasantosr
Copy link
Author

@arlyon I've updated the PR removing the From<String> impl, but tests are failing for another reason (I've run them locally and they're fine)

@lasantosr
Copy link
Author

Hey @arlyon it's been a while, ¿could you have a look at this and the other next PRs?

@mzeitlin11
Copy link
Collaborator

Sorry for the delay @lasantosr, if Alexander doesn't get to it by next weekend I should have more time then to review. For now, I just reran the CI jobs to see if happens again

@mzeitlin11
Copy link
Collaborator

Looks like the weird errors went away, now just a clippy one that if you don't see locally probably happens because of a different clippy version running in CI

@lasantosr lasantosr force-pushed the feature/keep-unknown-variants branch from d4583c2 to feb39af Compare October 26, 2024 16:37
@lasantosr
Copy link
Author

Thanks @mzeitlin11!

It was by bad, it's fixed now.

Copy link
Collaborator

@mzeitlin11 mzeitlin11 left a comment

Choose a reason for hiding this comment

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

Thanks @lasantosr!

@mzeitlin11 mzeitlin11 merged commit 40caf23 into arlyon:next Nov 2, 2024
15 checks passed
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.

3 participants