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: adds support for emitting default scalar values in json #919

Merged
merged 4 commits into from
Sep 24, 2023

Conversation

wgottschalk
Copy link
Contributor

Adds a new option for emitting default fields to support #907

Note that I only added support for the toJSON method. I designed the option to be easily modified to accept an array of fields similar to snakeToCamel. I didn't support the encoders or extensions because I'm not sure how to properly test those methods. If there's any code samples, I'd be happy to take a look and implement that

@wgottschalk wgottschalk changed the title adds support for emitting default scalar values in json feat: adds support for emitting default scalar values in json Sep 11, 2023
@stephenh
Copy link
Owner

stephenh commented Sep 18, 2023

Hey @wgottschalk ! Thanks for the PR! Apologies for the delay reviewing, have just been busy...

Fwiw I'm kind of dumb-founded that this so-obvious-in-retrospect slice point for "output default or not" already existed...it's been awhile since I was deep in this code, but 🤷 , that's why we have smart & motivated contributors like yourself I guess! 🎉

Looks like maybe there is a small options test failing, just b/c of the new option you added, I assume.

Also do you mind running format/prettier on integration/emit-default-values-json/emit-default-values-test.ts?

Otherwise it looks great, and I'll merge once the build is passing!

@stephenh
Copy link
Owner

stephenh commented Sep 18, 2023

I designed the option to be easily modified to accept an
array of fields similar to snakeToCamel.

Nice, good idea to follow that existing approach.

I didn't support the encoders or extensions because I'm not sure
how to properly test those methods.

I think the encoders are probably fine to leave as-is, b/c they output a binary format that a) no one sees, and b) b/c it's binary most implementations pretty strictly follow it, vs. the JSON encoding where it's got this "someone people want defaults/some don't" nuance.

By extensions, what are you referring to? Sorry, just not following/remembering.

@wgottschalk
Copy link
Contributor Author

Looks like maybe there is a small options test failing, just b/c of the new option you added, I assume.
Also do you mind running format/prettier on integration/emit-default-values-json/emit-default-values-test.ts?

Just updated the snapshots and ran formatting on the directory.

@wgottschalk
Copy link
Contributor Author

By extensions, what are you referring to? Sorry, just not following/remembering.

There's a function called generateExtension which has a call to notDefaultChecked

function generateExtension(ctx: Context, message: DescriptorProto | undefined, extension: FieldDescriptorProto) {

ts-proto/src/main.ts

Lines 1617 to 1624 in df0c596

} else if (isScalar(extension) || isEnum(extension)) {
chunks.push(code`
if (${notDefaultCheck(ctx, extension, message?.options, "value")}) {
const writer = ${Writer}.create();
${writeSnippet("value")};
encoded.push(writer.finish());
}
`);

@stephenh
Copy link
Owner

Looks great @wgottschalk , thanks!

@stephenh stephenh merged commit 01f529f into stephenh:main Sep 24, 2023
6 checks passed
stephenh pushed a commit that referenced this pull request Sep 24, 2023
# [1.158.0](v1.157.1...v1.158.0) (2023-09-24)

### Features

* adds support for emitting default scalar values in json ([#919](#919)) ([01f529f](01f529f))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.158.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

2 participants