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

Properly skip request placeholder types #577

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Properly skip request placeholder types #577

wants to merge 1 commit into from

Conversation

nfx
Copy link
Contributor

@nfx nfx commented Aug 10, 2023

This PR adds a way to list types that are not mere request placeholders, so that Python SDK keyed args UX is not cluttered with the types of no use.

To be verified after databricks/databricks-sdk-py#264 is merged

This PR adds a way to list types that are not mere request placeholders, so that Python SDK keyed args UX is not cluttered with the types of no use.
@nfx nfx added codegen Related to code generation do-not-merge labels Aug 10, 2023
@nfx nfx requested a review from mgyucht August 10, 2023 13:02
Comment on lines +210 to +219
for _, e := range svc.Package.types {
for _, f := range e.fields {
if f.Entity == request {
// this type is not only a request placeholder, but is also
// part of other type definitions.
isOnlyUsedAsRequest = false
}
}
}
request.IsRequestOnly = isOnlyUsedAsRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this need to be done as a postprocessing step? The order in which requests are defined is arbitrary AFAIK, so this could accidentally set types as request only if their uses are not yet processed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't this need to be done as a postprocessing step?

it could be in NonPlaceholderTypes() method. may add some false positives, but will try that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen Related to code generation do-not-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants