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

chore: Add support for Narwhals Binary dtype in serialize_dtype #1928

Merged

Conversation

FBruzzesi
Copy link
Contributor

@FBruzzesi FBruzzesi commented Mar 19, 2025

Description

Narwhals#2243 is going to introduce introduced Binary dtype, this PR adds support for that by serializing Binary to "binary"

@schloerke schloerke requested a review from Copilot March 24, 2025 14:25
@schloerke
Copy link
Collaborator

Thank you!

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for Narwhals' new Binary dtype by updating serialization logic.

  • Added "binary" to the list of allowed dtypes in FrameDtypeSubset.
  • Introduced a new conditional branch in serialize_dtype to convert nw.Binary to "binary".

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
shiny/render/_data_frame_utils/_types.py Added "binary" to the FrameDtypeSubset list.
shiny/render/_data_frame_utils/_tbl_data.py Added conditional branch to serialize nw.Binary as "binary".
Comments suppressed due to low confidence (1)

shiny/render/_data_frame_utils/_tbl_data.py:211

  • Consider adding tests to verify that the new Binary dtype branch in serialize_dtype serializes values correctly.
elif hasattr(nw, "Binary") and isinstance(

@schloerke schloerke merged commit 98858fd into posit-dev:main Mar 24, 2025
54 checks passed
@FBruzzesi FBruzzesi deleted the chore/support-for-narwhals-binary-dtype branch March 26, 2025 08:10
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