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.

Sign up for GitHub

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

Jump to bottom

feat: Initialize lists with map #387

Merged
merged 5 commits into from
Nov 15, 2021
Merged

feat: Initialize lists with map #387

merged 5 commits into from
Nov 15, 2021

Conversation

webmaster128
Copy link
Collaborator

@webmaster128 webmaster128 commented Nov 14, 2021 •
edited
Loading

This uses map to initialize lists, such that all cases can be handled within one assignment. if (object.ids !== undefined && object.ids !== null) { was replaced with object.ids ?? [] to save nesting depth and code size.

@webmaster128
Copy link
Collaborator Author

Any idea why CI is failing here? The problematic "Example 6" was added here in #370. The other integration/*/google/protobuf/timestamp.ts alle have the older version of the comment. Why did this not fail in the PR #370 or on main but now?

@stephenh stephenh mentioned this pull request Nov 15, 2021
Merged
@stephenh
Copy link
Owner

@webmaster128 yeah, I'm not really sure how that timestamp.ts change got through that other PR... like it was only a month or so ago, and I haven't changed any of the lint/CI setup in quite awhile.

I merged #388 so am personally pretty willing to procrastinate on figuring out true root cause + better yet moving to a better protoc setup. :-) But if that's something you'd like to improve, that'd be great, we definitely need something more deterministic than the current setup.

This PR also looks great! Thanks!

@webmaster128 webmaster128 force-pushed the initialize-list-with-map branch from 78f4a3e to 869b65e Compare November 15, 2021 18:08
webmaster128
webmaster128 commented Nov 15, 2021
View reviewed changes
src/main.ts
@@ -1249,7 +1225,7 @@ function generateFromPartial(ctx: Context, fullName: string, messageDesc: Descri
chunks.push(code`message.${fieldName} = ${readSnippet(`object.${fieldName}`)};`);
chunks.push(code`} else {`);
const fallback = isWithinOneOf(field) ? 'undefined' : defaultValue(ctx, field);
chunks.push(code`message.${fieldName} = ${fallback}`);
chunks.push(code`message.${fieldName} = ${fallback};`);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is unrelated and simply here to increase consistency between generateFromPartial and generateFromJson.

@webmaster128
Copy link
Collaborator Author

Rebased. When it passes CI, I'm happy.

@webmaster128 webmaster128 merged commit 200e674 into stephenh:main Nov 15, 2021
@webmaster128 webmaster128 deleted the initialize-list-with-map branch November 15, 2021 20:50
stephenh pushed a commit that referenced this pull request Nov 15, 2021
@semantic-release-bot
chore(release): 1.86.0 [skip ci]
9e746e0 
# [1.86.0](v1.85.0...v1.86.0) (2021-11-15)

### Features

* Initialize lists with map ([#387](#387)) ([200e674](200e674))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.86.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
Reviewers

@stephenh stephenh stephenh approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@webmaster128 @stephenh