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

Automate amqp code generation and add test to catch drift #1827

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ddelnano
Copy link
Member

@ddelnano ddelnano commented Jan 24, 2024

Summary: Automate amqp code generation and add test to catch drift

This is a work in progress change to implement what was described here.

Relevant Issues: #1820 (comment)

Type of change: /kind cleanup

Test Plan: diff_file rules fail until linting issues are resolved

screen

@vihangm
Copy link
Member

vihangm commented Jan 24, 2024

I think it's okay to skip linting for autogenerated files. Might be worth adding them to the exclusion list and calling this done.

@ddelnano ddelnano force-pushed the ddelnano/reduce-amqp-code-gen-manual branch from 49f3a87 to 55b3e53 Compare January 24, 2024 07:34
@vihangm
Copy link
Member

vihangm commented Jan 24, 2024

I think if you reduce the tab/spaces in the returns for gen_method_enum_select_case, gen_method_enum_declr etc to the expected output, the generated code is probably mostly formatted as expected.

There's a lot of them though, so unsure if it's worth the trouble.

@ddelnano
Copy link
Member Author

I was expecting the diff 'as is' to be pretty nasty, which is why I tried to keep the code formatting intact initially. I'll see how difficult it is to get the code generation to output relatively formatted code.

@ddelnano
Copy link
Member Author

ddelnano commented Jan 25, 2024

@vihangm my latest approach gets the files formatted with minor whitespace differences. It required quite a few changes to the amqp code generation file. The changes to amqp_code_gen.py could be cleaner, but I wanted to get a feel for how many lines needed to be touched to make the diff reasonable. It required enough whitespace changes that my preference would be to stick with the initial approach that formatted the output files according to our clang-format config.

Let me know what you think. I'm holding off on fixing the amqp_code_gen_test.py tests until we have consensus on this PR's direction.

@vihangm
Copy link
Member

vihangm commented Jan 25, 2024

Given that you have spent a bunch of time here, I trust your judgement. If you think running clang-format as part of the generation isn't too much trouble, that approach seems fine too. I was worried that running the linter would be more painful and hence suggested skipping lint/updating the code generation. :)

@ddelnano ddelnano force-pushed the ddelnano/reduce-amqp-code-gen-manual branch from e1d37a4 to db13912 Compare January 26, 2024 06:17
@ddelnano ddelnano marked this pull request as ready for review January 26, 2024 06:18
@ddelnano ddelnano requested review from a team as code owners January 26, 2024 06:18
@ddelnano
Copy link
Member Author

Sounds good 👍. I've updated the branch based on the initial approach and it's ready for review now.

@ddelnano ddelnano force-pushed the ddelnano/reduce-amqp-code-gen-manual branch from 7dc1bab to ada6662 Compare January 26, 2024 14:59
ddelnano added 3 commits March 5, 2024 07:27
Signed-off-by: Dom Del Nano <[email protected]>
(cherry picked from commit 64b67a72291d4ca48c54b9c26b47c73fb8f25b5b)
@ddelnano ddelnano force-pushed the ddelnano/reduce-amqp-code-gen-manual branch from ada6662 to 08d1604 Compare March 5, 2024 07:30
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.

2 participants