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(java): Line up protobuf type handling with our other generators #6322

Merged
merged 6 commits into from
Mar 6, 2025

Conversation

ajgateno
Copy link
Member

@ajgateno ajgateno commented Mar 6, 2025

We were using longs for uints, when we should have been using ints

@ajgateno ajgateno marked this pull request as ready for review March 6, 2025 21:08
@ajgateno ajgateno requested a review from dsinghvi as a code owner March 6, 2025 21:08
@@ -183,7 +183,7 @@ public String visitLong() {

@Override
public String visitUint() {
return "Long";
return "Integer";
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should name this distinct even though it still maps to an int. That way if a user has an undiscriminated union with both there's no conflict. Otherwise the following type would have two things named with Integer:

types:
  Number:
    discriminated: false
    union:
      - uint
      - integer

Copy link
Member

Choose a reason for hiding this comment

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

Ah ignore me, I was thinking this would be used in the union generator, but this detail likely isn't relevant for Java!

Copy link
Member

@amckinney amckinney left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link

github-actions bot commented Mar 6, 2025

@ajgateno ajgateno merged commit 030691f into main Mar 6, 2025
43 checks passed
@ajgateno ajgateno deleted the alberto/support-protobuf-types-java branch March 6, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants