Use .IsLegacyEmptyRequest instead of checking that .Request is nil in codegen#3098
Merged
shreyas-goenka merged 5 commits intomainfrom Jun 26, 2025
Merged
Conversation
parthban-db
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
parthban-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
parthban-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
reviewed
Jun 23, 2025
Contributor
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
Can you revert the changes to .github and remove tagging.py?
We use a different location for tagging.py and don't use the NEXT_CHANGELOG detector.
parthban-db
force-pushed
the
parthban-db/request-message-always-defined
branch
from
2f15d60 to
86e842a
Compare
parthban-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
parthban-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
parthban-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| func newList() *cobra.Command { | ||
| cmd := &cobra.Command{} | ||
|
|
||
| // TODO: short flags |
Contributor
There was a problem hiding this comment.
Why are these comments being added? Do you need to rebase?
Contributor
Author
There was a problem hiding this comment.
Because the TODO depends on the request being null, and it is never null from now on. It is just not rendered for legacy requests.
Line 149 in 37d2683
Contributor
shreyas-goenka
left a comment
shreyas-goenka
left a comment
There was a problem hiding this comment.
The template changes look good to me. We need to fix the // TODO being added though.
| var {{.CamelName}}Overrides []func( | ||
| *cobra.Command, | ||
| {{- if .Request }} | ||
| {{- if not .IsLegacyEmptyRequest }} |
Contributor
There was a problem hiding this comment.
Can you explain the semantics of .IsLegacyEmptyRequest in the PR description? For which fields will it be set.
parthban-db
force-pushed
the
parthban-db/request-message-always-defined
branch
from
37d2683 to
379fe40
Compare
parthban-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
parthban-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
approved these changes
Jun 25, 2025
shreyas-goenka
changed the title
.IsLegacyEmptyRequest instead of checking that .Request is nil in codegen
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Changes because of always defining request messages. There is a bug in the Code Generator that sets the request to nil if it has no fields. As we fix the generator, it changes the generated code. To keep the generated code the same, we need to add exceptions for already existing requests.
IsLegacyEmptyRequest returns true if the request has no fields and, for the above reason, is not rendered in the SDK.
This PR also removes TODO from the template, which adds noise to the diff. Also, we don't need it to remind us to implement short flags.
Why
We made a few changes in the Code generator that ensure that the request can never be nil. However, we cannot add the request to the existing interface. So, adding an exception for legacy commands.
Tests
Existing CI. Manually verified this change, only changed a few comments.