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 in rule for repeated int32 #1240

Merged
merged 3 commits into from
Jan 29, 2025
Merged

Fix in rule for repeated int32 #1240

merged 3 commits into from
Jan 29, 2025

Conversation

mortezaPRK
Copy link
Contributor

@mortezaPRK mortezaPRK commented Jan 27, 2025

Fix generating lookup for repeated.items.int32.in

This PR fixes the in field validation for repeated int32 fields.

The current generated code doesn't emit the lookup map e.g.

// message DoSomethingRequest {
//   repeated int32 field_with_int32_type = 4 [ (validate.rules).repeated.items.int32 = { in: [25,50] } ];
// }

// Generated code doesn't have this section
var _DoSomethingRequest_FieldWithInt32Type_InLookup = map[int32]struct{}{
	25:  {},
	50:  {},
}

I also added a Test cases for both int32 and int64

@CLAassistant
Copy link

CLAassistant commented Jan 27, 2025

CLA assistant check
All committers have signed the CLA.

@mortezaPRK mortezaPRK marked this pull request as ready for review January 27, 2025 16:32
@rodaine
Copy link
Member

rodaine commented Jan 27, 2025

Hi @mortezaPRK thanks for the patch! Looks like the lookup was also not generated in the C++ codegen either.

@mortezaPRK
Copy link
Contributor Author

Hey @rodaine

Unfortunately I can't run the tests locally (I tried with Docker and it doesn't work), and I have no idea how C++ code looks like.

Should I remove the test cases I added? I found this PR which is doing something similar tho.

@nicksnyder
Copy link
Member

@mortezaPRK maybe the best path forward is to split this into 2 different PRs. Keep the fixes for Go in this PR and remove the tests you added so we can get this merged as an incremental improvement. Then open a separate PR to add the tests (which will still fail, but will be a reminder that there is a gap in C++ codegen).

@mortezaPRK
Copy link
Contributor Author

mortezaPRK commented Jan 29, 2025

@nicksnyder Agree. I updated the PR.

I'll create another PR for the test cases

@rodaine rodaine changed the title Fix in rule for repeatet int32 Fix in rule for repeated int32 Jan 29, 2025
Copy link
Member

@rodaine rodaine left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks again

@rodaine rodaine merged commit 94f2b56 into bufbuild:main Jan 29, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants