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: Struct wrap/unwrap snake case #579 #588

Merged
merged 3 commits into from
Jun 5, 2022

Conversation

Celend
Copy link
Contributor

@Celend Celend commented Jun 3, 2022

fixes #579

please review :D

@stephenh
Copy link
Owner

stephenh commented Jun 3, 2022

Nice! This lgtm @Celend .

To make sure we don't break this in the future, can you add a struct example to our snake-case test:

https://github.com/stephenh/ts-proto/tree/main/integration/simple-snake

And use the yarn proto2bin/bin2ts commands to generate + check-in code that includes the now-fixed struct logic?

Thanks!

@Celend
Copy link
Contributor Author

Celend commented Jun 3, 2022

there are two different types of snake case about struct: with/without oneof union, how do I test all cases? @stephenh

create multiple folders in integration?

@stephenh
Copy link
Owner

stephenh commented Jun 3, 2022

@Celend yeah, if we needed to test both snake + w/union and snake + w/o union (afaiu those are the two combinations), then we'd need two separate test directories.

Fwiw if we can reproduce just one of those two in the existing simple-snake test, I'm good with calling that good enough, but also yeah ideally we'd 1) add a struct to the existing simple-snake test, and 2) copy/paste oneof-unions and make it oneof-unions-snake, and then we'd have both combinations covered.

(If that sounds right, I'm kind of skimming/reading quickly, so feel free to use better test names/etc., if I'm not 100% right on which two cases we need to cover.)

@Celend
Copy link
Contributor Author

Celend commented Jun 3, 2022

@stephenh It can't be done in one integration test because it's control by parameters.txt, so I create a new test.

please take a look, the test is still running and slow😭

If we want test two cases in one integration test, I think we need something like fixture that can use multiple parameters.txt to running one test suite, but also heavier overhead.

@stephenh
Copy link
Owner

stephenh commented Jun 5, 2022

Looks great, thanks @Celend !

@stephenh stephenh merged commit 33f89bf into stephenh:main Jun 5, 2022
stephenh pushed a commit that referenced this pull request Jun 5, 2022
## [1.115.4](v1.115.3...v1.115.4) (2022-06-05)

### Bug Fixes

* Struct wrap/unwrap snake case [#579](#579) ([#588](#588)) ([33f89bf](33f89bf))
@stephenh
Copy link
Owner

stephenh commented Jun 5, 2022

🎉 This PR is included in version 1.115.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Celend Celend deleted the fix-struct-wrap-snake-case branch June 6, 2022 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrap/unwrap function in google.protobuf.struct doesn't use snake case when --ts_proto_opt=snakeToCamel=false
2 participants