Skip to content

refactor: Clean up table feature parsing with strum-0.28#1925

Open
scovich wants to merge 3 commits intodelta-io:mainfrom
scovich:strum-476-followup
Open

refactor: Clean up table feature parsing with strum-0.28#1925
scovich wants to merge 3 commits intodelta-io:mainfrom
scovich:strum-476-followup

Conversation

@scovich
Copy link
Copy Markdown
Collaborator

@scovich scovich commented Feb 23, 2026

What changes are proposed in this pull request?

Now that Peternator7/strum#476 merged, we can use From/Into instead of the bespoke IntoTableFeature trait.

How was this change tested?

Existing unit tests.

@github-actions github-actions bot added the breaking-change Public API change that could cause downstream compilation failures. Requires a major version bump. label Feb 23, 2026
@scovich scovich removed the breaking-change Public API change that could cause downstream compilation failures. Requires a major version bump. label Feb 23, 2026
@scovich
Copy link
Copy Markdown
Collaborator Author

scovich commented Feb 23, 2026

Note: We shouldn't merge this unless/until we're ready to require strum-0.28 (it had been pinned at 0.27, not sure why)

@scovich scovich added the merge hold Don't allow the PR to merge label Feb 23, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.07%. Comparing base (44055c9) to head (7e42748).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1925      +/-   ##
==========================================
- Coverage   86.07%   86.07%   -0.01%     
==========================================
  Files         139      139              
  Lines       41286    41280       -6     
  Branches    41286    41280       -6     
==========================================
- Hits        35538    35532       -6     
  Misses       4219     4219              
  Partials     1529     1529              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

I'm loving the cleanup!

Comment thread kernel/Cargo.toml
serde = { version = "1", features = ["derive", "rc"] }
serde_json = "1"
strum = { version = "0.27", features = ["derive"] }
strum = { version = "0.28", features = ["derive"] }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It was pinned because of an Arrow update: https://github.com/delta-io/delta-kernel-rs/pull/885/changes#diff-45b9953e854edc633b513d9ce5dbb79628995bd5fc7f8880372d76bb4fffa0fdR48

Arrow has a PR open to bump to 0.28 as well: apache/arrow-rs#9471

If it compiles, it works, right? 🤷

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Does the arrow update allow or force 0.28? Seems like = would force it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would force arrow to compile with that yeah, but as long as there's not a boundary between two crates where they try and exchange data types from a shared dependency, they can use different versions of that dependency.

Comment thread kernel/Cargo.toml
serde = { version = "1", features = ["derive", "rc"] }
serde_json = "1"
strum = { version = "0.27", features = ["derive"] }
strum = { version = "0.28", features = ["derive"] }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would force arrow to compile with that yeah, but as long as there's not a boundary between two crates where they try and exchange data types from a shared dependency, they can use different versions of that dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge hold Don't allow the PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants