Skip to content

Conversation

@avallete
Copy link
Member

What kind of change does this PR introduce?

Bug fix, feature, docs update, ...

What is the current behavior?

Please link any relevant issues here.

What is the new behavior?

Feel free to include screenshots if it includes visual changes.

Additional context

Add any other context or screenshots.

@avallete avallete requested a review from soedirgo as a code owner November 18, 2025 17:04
Copy link
Member Author

@avallete avallete left a comment

Choose a reason for hiding this comment

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

Looking up why the filter was needed I've found that we indeed need to filter over types based on included schemas (because for them we include all system schemas conflicting with user provided filters). The other places are filtered at introspection level.

Comment on lines +27 to +28
// We always include system schemas, so we need to filter out types that are not in the included schemas
.filter((type) => type.attributes.length > 0 && schemasNames.has(type.schema))
Copy link
Member Author

Choose a reason for hiding this comment

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

note

This is the only place where the inclusion/exclusion filters from the user are bypassed and must be manually applied. Matching typescript generation:

if (type.schema in introspectionBySchema) {
if (type.enums.length > 0) {
introspectionBySchema[type.schema].enums.push(type)
}
if (type.attributes.length > 0) {
introspectionBySchema[type.schema].compositeTypes.push(type)
}
}

@soedirgo
Copy link
Member

soedirgo commented Nov 21, 2025

LGTM, but just confirming that this is only for perf, there's no bug on the target PR? Nvm I get what's happening - for everything except composite types the schema filter is redundant

@avallete avallete merged commit b0478c3 into feature/python-generator Nov 21, 2025
6 checks passed
@avallete avallete deleted the fix/non-included-schemas-filters branch November 21, 2025 11:22
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.

3 participants