Skip to content

python: Consolidate schema types #260

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

Merged
merged 3 commits into from
Feb 28, 2025
Merged

Conversation

gasmith
Copy link
Contributor

@gasmith gasmith commented Feb 28, 2025

This change didn't go as far as I thought it would when I wrote the ticket.

It turns out that the (message_encoding + Schema) we use for channels is really a (message_encoding + Optional[Schema]) because channels can be schema-less. The same is not true for service request/response schemas, which are mandatory.

Changes here:

  • Dropped SchemaDefinition.
  • The Channel constructor takes a message encoding and a Schema.
    • The message encoding is optional (and ignored) if the schema is a JSON schema.
    • The schema must be specified as a keyword argument, but may be None for schemaless.
  • Plumbed Schema through the BaseChannel constructor.
  • Removed the rejection of schemas with empty strings as encoding/data. If we really want to be that pedantic, we should do it in rust too.

I'm not thrilled with the result, but I thought I'd throw it up for review anyway. Feel free to nack.

@gasmith gasmith self-assigned this Feb 28, 2025
Copy link

linear bot commented Feb 28, 2025

@gasmith gasmith marked this pull request as ready for review February 28, 2025 19:08
Copy link
Contributor

@bryfox bryfox left a comment

Choose a reason for hiding this comment

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

I think this is an improvement. I started to write more feedback, but your added alternative seems to address this.

Removed the rejection of schemas with empty strings as encoding/data.

👍 ...I think this was a holdover from an early python version; we should be consistent, and mcap allows empty schemas.

@gasmith gasmith requested review from bryfox and eloff February 28, 2025 19:35
@gasmith gasmith merged commit abed152 into main Feb 28, 2025
36 checks passed
@gasmith gasmith deleted the gasmith/fg-10612-py-schema-types branch February 28, 2025 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants