arlyon / async-stripe

Async (and blocking!) Rust bindings for the Stripe API

https://payments.rs

Apache License 2.0

454 stars 129 forks source link

Fix issue with missing rename of Self_ #490

Closed jwiesler closed 7 months ago

jwiesler commented 8 months ago

Summary

See #489.

The issue is that serde's snake case for enum variants differs from heck's snake case, especially for Self_ (and one more unexpected case, see the diff).

This was fixed by including the function used by serde, which is sadly not exported so I copy pasted it (serde is under MIT license). The issue here is obviously that serde may change the internal function but I think this is very unlikely since it breaks all parsing code in existence and serde is no longer in alpha :)

Checklist

[x] ran cargo make fmt
[x] using conventional commits to hightlight user-facing fixes and features

jwiesler commented 8 months ago

@arlyon

arlyon commented 8 months ago

Running CI now, thanks!

arlyon commented 8 months ago

Closes #489

jwiesler commented 8 months ago

I think we should probably just strip the trailing underscore or rework the Self case to use raw identifiers like r#Self

For Self this might solve the problem, but as you can see above the two implementations even differ in other places, this is just waiting for bugs to appear. There is no testing to make sure the resulting renames generated by serde match the open api definition.

jwiesler commented 7 months ago

So just to make sure I've mentioned everything: The two implementations differ in:

Trailling _
_ before upper case character in the name We could patch the implementation from heck, but at this point this patch is as large as just copying the implementation from serde.

Furthermore the code relies on the two snake cases (what serde does and what we do to determine whether a rename is needed) aligning, otherwise we will output wrong renames or unneeded renames.

@arlyon could you please give an update on this or let someone else review this mr?

arlyon commented 7 months ago

Did some more digging and happy to merge this. Thanks!

arlyon commented 7 months ago

Apologies for the delay, we have a few very kind people who participate sometimes but usually when I am busy PR reviews stall. Thank you for taking the time.

jwiesler commented 7 months ago

Is there anything broken with the release pipeline? The crates.io version is stuck at 0.31 and we are at 0.33 in this repository already since three weeks? @arlyon

arlyon commented 7 months ago

Yeah, I need to manually push them. One of many things with this repo I've been meaning to get around to.