Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

feat: Add numeric enum support. #1743

Merged
merged 4 commits into from
Aug 22, 2022
Merged

feat: Add numeric enum support. #1743

merged 4 commits into from
Aug 22, 2022

Conversation

blakeli0
Copy link
Contributor

@blakeli0 blakeli0 commented Jul 23, 2022

Add numeric enum support based on go/actools-numeric-enum-impl.
Add a new overloaded toBody method that support serializing request object to Json with numeric enums.

@blakeli0 blakeli0 requested review from a team as code owners July 23, 2022 03:24
@@ -79,7 +79,7 @@ public ResponseT parse(Reader httpContent, TypeRegistry registry) {
/* {@inheritDoc} */
@Override
public String serialize(ResponseT response) {
return ProtoRestSerializer.create(defaultRegistry).toJson(response);
return ProtoRestSerializer.create(defaultRegistry).toJson(response, false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vam-google I couldn't find any usage of this method, is this method only used by tests? It also feels strange to me that a response parser need to serialize an object to String.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe it is used only by tests, but that also counts as "usage", right? I think this "serialize" interface thing predates my time on rest transport, so I just had to keep it there to not break things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I think passing false is appropriate then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, we should probably add some comments to indicate this public method is only used in tests to set up things? Or we can make it better by using a different way to serialize object here, so that we can remove this test only method?

@blakeli0 blakeli0 requested review from vam-google and meltsufin July 23, 2022 03:29
@blakeli0 blakeli0 requested a review from vchudnov-g July 27, 2022 16:03
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

Please make it non-breaking. BetaApi is mainly to convey a message that API is not stable yet and may change only if there is no better way. Here it seems making it non-breaking is simple and cheap, so it seems like abetter approach than taking a breaking change.

@blakeli0 blakeli0 requested a review from vam-google July 29, 2022 15:08
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM, but lets resolve the field name question before submitting

@@ -79,7 +79,7 @@ public ResponseT parse(Reader httpContent, TypeRegistry registry) {
/* {@inheritDoc} */
@Override
public String serialize(ResponseT response) {
return ProtoRestSerializer.create(defaultRegistry).toJson(response);
return ProtoRestSerializer.create(defaultRegistry).toJson(response, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe it is used only by tests, but that also counts as "usage", right? I think this "serialize" interface thing predates my time on rest transport, so I just had to keep it there to not break things.

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM sinc i'll be OOO, but please resolve the field name quesiton first.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

88.9% 88.9% Coverage
0.0% 0.0% Duplication

@blakeli0
Copy link
Contributor Author

LGTM sinc i'll be OOO, but please resolve the field name quesiton first.

Added the fieldName argument to the overloaded method.

@blakeli0 blakeli0 merged commit 3f7628e into main Aug 22, 2022
@blakeli0 blakeli0 deleted the support_numeric_enum branch August 22, 2022 22:50
gcf-merge-on-green bot pushed a commit that referenced this pull request Aug 23, 2022
🤖 I have created a release *beep* *boop*
---


## [2.19.0](v2.18.7...v2.19.0) (2022-08-22)


### Features

* Add numeric enum support. ([#1743](#1743)) ([3f7628e](3f7628e))


### Bug Fixes

* **deps:** update dependency com.google.auth:google-auth-library-credentials to v1.10.0 ([#1768](#1768)) ([3f2188d](3f2188d))
* **deps:** update dependency com.google.auth:google-auth-library-credentials to v1.9.0 ([#1765](#1765)) ([103db3c](103db3c))
* **deps:** update dependency com.google.auth:google-auth-library-oauth2-http to v1.10.0 ([#1769](#1769)) ([0b1eb92](0b1eb92))
* **deps:** update dependency com.google.auth:google-auth-library-oauth2-http to v1.9.0 ([#1766](#1766)) ([2677f07](2677f07))
* **deps:** update dependency com.google.code.gson:gson to v2.9.1 ([#1757](#1757)) ([ea2a075](ea2a075))
* **deps:** update dependency com.google.protobuf:protobuf-bom to v3.21.5 ([#1772](#1772)) ([d7a48d1](d7a48d1))
* **deps:** update dependency io.grpc:grpc-bom to v1.48.1 ([#1763](#1763)) ([e5e4232](e5e4232))
* **deps:** update dependency org.graalvm.sdk:graal-sdk to v22.2.0 ([#1740](#1740)) ([ded44a6](ded44a6))
* **deps:** update dependency org.mockito:mockito-core to v4.7.0 ([#1774](#1774)) ([29678c8](29678c8))
* **deps:** update dependency org.threeten:threetenbp to v1.6.1 ([#1773](#1773)) ([d2c84e6](d2c84e6))
* **test:** testThrottlingBlocking flakyness fix ([#1775](#1775)) ([e69393c](e69393c))


### Documentation

* explaining UNIX environment is required ([#1760](#1760)) ([1d31e90](1d31e90))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
suztomo pushed a commit to googleapis/sdk-platform-java that referenced this pull request Dec 16, 2022
🤖 I have created a release *beep* *boop*
---


## [2.19.0](googleapis/gax-java@v2.18.7...v2.19.0) (2022-08-22)


### Features

* Add numeric enum support. ([#1743](googleapis/gax-java#1743)) ([fa87970](googleapis/gax-java@fa87970))


### Bug Fixes

* **deps:** update dependency com.google.auth:google-auth-library-credentials to v1.10.0 ([#1768](googleapis/gax-java#1768)) ([6008b0d](googleapis/gax-java@6008b0d))
* **deps:** update dependency com.google.auth:google-auth-library-credentials to v1.9.0 ([#1765](googleapis/gax-java#1765)) ([ba597b4](googleapis/gax-java@ba597b4))
* **deps:** update dependency com.google.auth:google-auth-library-oauth2-http to v1.10.0 ([#1769](googleapis/gax-java#1769)) ([db37c42](googleapis/gax-java@db37c42))
* **deps:** update dependency com.google.auth:google-auth-library-oauth2-http to v1.9.0 ([#1766](googleapis/gax-java#1766)) ([fe181b0](googleapis/gax-java@fe181b0))
* **deps:** update dependency com.google.code.gson:gson to v2.9.1 ([#1757](googleapis/gax-java#1757)) ([36a0136](googleapis/gax-java@36a0136))
* **deps:** update dependency com.google.protobuf:protobuf-bom to v3.21.5 ([#1772](googleapis/gax-java#1772)) ([c9a2a56](googleapis/gax-java@c9a2a56))
* **deps:** update dependency io.grpc:grpc-bom to v1.48.1 ([#1763](googleapis/gax-java#1763)) ([5535101](googleapis/gax-java@5535101))
* **deps:** update dependency org.graalvm.sdk:graal-sdk to v22.2.0 ([#1740](googleapis/gax-java#1740)) ([77f66d3](googleapis/gax-java@77f66d3))
* **deps:** update dependency org.mockito:mockito-core to v4.7.0 ([#1774](googleapis/gax-java#1774)) ([e192e0b](googleapis/gax-java@e192e0b))
* **deps:** update dependency org.threeten:threetenbp to v1.6.1 ([#1773](googleapis/gax-java#1773)) ([b9e4767](googleapis/gax-java@b9e4767))
* **test:** testThrottlingBlocking flakyness fix ([#1775](googleapis/gax-java#1775)) ([9aef46a](googleapis/gax-java@9aef46a))


### Documentation

* explaining UNIX environment is required ([#1760](googleapis/gax-java#1760)) ([1555924](googleapis/gax-java@1555924))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants