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

Fix issue with missing rename of Self_ #490

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

jwiesler
Copy link
Collaborator

@jwiesler jwiesler commented Jan 29, 2024

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

@jwiesler
Copy link
Collaborator Author

@arlyon

@arlyon
Copy link
Owner

arlyon commented Feb 2, 2024

Running CI now, thanks!

@arlyon
Copy link
Owner

arlyon commented Feb 2, 2024

Closes #489

@arlyon arlyon linked an issue Feb 2, 2024 that may be closed by this pull request
Copy link
Owner

@arlyon arlyon left a comment

Choose a reason for hiding this comment

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

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

src/resources/generated/issuing_card.rs Show resolved Hide resolved
@jwiesler
Copy link
Collaborator Author

jwiesler commented Feb 2, 2024

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
Copy link
Collaborator Author

jwiesler commented Feb 15, 2024

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
Copy link
Owner

arlyon commented Feb 15, 2024

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

@arlyon arlyon merged commit a91ac97 into arlyon:master Feb 15, 2024
16 checks passed
@arlyon
Copy link
Owner

arlyon commented Feb 15, 2024

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
Copy link
Collaborator Author

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
Copy link
Owner

arlyon commented Feb 16, 2024

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

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.

Parsing issue with CreateInvoiceAutomaticTaxLiabilityType
2 participants