fix: no panic on unknown version#5111
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5111 +/- ##
==========================================
+ Coverage 81.69% 81.71% +0.01%
==========================================
Files 340 340
Lines 138875 138910 +35
Branches 138875 138910 +35
==========================================
+ Hits 113456 113509 +53
+ Misses 21679 21665 -14
+ Partials 3740 3736 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| /// A parsed Lance library version with major, minor, and patch components. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
| pub struct LanceVersion { |
There was a problem hiding this comment.
why do we want a new LanceVersion instead of just the current WriterVersion? If we are doing this, I think we should just use https://docs.rs/semver/latest/semver/
There was a problem hiding this comment.
WriterVersion is the version of some arbitrary library. It might be lance, it might be another library. And if it's another library, it might not use a semantic versioning scheme.
LanceVersion is the version of the Lance Rust library. We guarantee it's semantically versioned. We could pull in semver for this, but it's also not clear we really care to add a dependency for this stuff. If you think it's worth it, feel free.
There was a problem hiding this comment.
I think the main benefit is that it handles all sorts of pre release string formats so we don't need a custom parser anymore. Originally Xuanwo proposed it but I didn't add because it breaks backwards compatibility, but since we are already refactoring it here we could just do it along the way. But I can put another PR for it after this along they way to make the pre release forward compatible. I will just hold off publishing it until this is merged.
Previously, version parsing was done manually with string splitting. This changes LanceVersion to wrap semver::Version, providing robust parsing and proper handling of pre-release versions like beta.1. Also renames lance_version() to lance_lib_version() to clarify that it refers to the library version, not the file format version. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
| impl WriterVersion { | ||
| /// Try to parse the version string as a semver string. Returns None if | ||
| /// not successful. | ||
| #[deprecated(note = "Use `lance_lib_version()` instead")] |
There was a problem hiding this comment.
I was thinking this could still be useful for non Lance library version as long as they are using semver. But this should be fine to deprecate for now.
There was a problem hiding this comment.
Yeah if we start doing carveouts for other libraries, we can add that back.
For more context, see: lance-format#5099 (reply in thread) --------- Co-authored-by: Claude <noreply@anthropic.com>
For more context, see: lance-format#5099 (reply in thread) --------- Co-authored-by: Claude <noreply@anthropic.com>
3 participants
For more context, see: #5099 (reply in thread)