fix: forward incompatibility of prerelease in writer version#5116
fix: forward incompatibility of prerelease in writer version#5116jackye1995 merged 2 commits intolance-format:mainfrom
Conversation
| // full semantic version by combining version, prerelease, and build_metadata. | ||
| // | ||
| // If absent, the version field is used as-is. | ||
| optional string prerelease = 3; |
There was a problem hiding this comment.
the original proposal was to just use a single classifier string, but that makes it hard to leverage the semver parser, so I made it aligned with the semver spec
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
Thanks for the fix. Does this supersede #5113 ?
There was a problem hiding this comment.
Are we baking in semver as a requirement for writers? Seems unnecessary for the format to be opinionated about that?
There was a problem hiding this comment.
It still works with arbitrary string. But in general it feels like recording the full semver is beneficial that we can know if the writer is a specific version, if it is the main release version or a specific beta version.
There was a problem hiding this comment.
Yeah I don't disagree with recording the full semver. More concerned that providing specific prerelease and build_metadata then we baking in semver concepts into the format. It seems like we could just have a field, version_extra or something like that where we put that segment of the version.
There was a problem hiding this comment.
yeah I went back and forth with it tbh.
Originally I just have a classifier field (that was originally what was proposed on Slack), then my internal debate is that for example I have a 1.2.3-beta.2 string, I can choose to store beta.2 in classifier, and I just split by - string to get that split. But what if there is a 1.2.3+build.abcde string which is still semver in the future, then it does not work.
I also thought about storing -beta.2 in the classifier, but then we need a parser to seek to the first non-version position of the string and then split there. It feels like a bit of an overkill, given for the Lance library it is basically always semver. Even if we have some internal versions in the future, it probably still makes sense to follow the semver part and just have -internal.2 or leverage build metadata to store internal info, so we can easily infer the ordering of different versions.
So that was my thoughts to arrive at this state.
I guess having a single classifier/version_extra field would also work, let me know which you prefer.
There was a problem hiding this comment.
Yeah I was thinking you could just parse semver as str_concat(version, version_extra).
There was a problem hiding this comment.
The problem is I think the reverse, how do you move from a version string to version and version_extra. It is not clear to me if for example if I have 1.0-beta.1, this is not a standard semver, does it mean I need to put everything to version, or split it to 1.0 and -beta.1.
The current approach provides a clear rule that (1) if it is semver, then can leverage those additional fields, (2) if it is not semver, everything is still stored in the version string.
If we want to move to just a version_extra, looks like we will define something like if it starts with 3 numbers connected with 2 dots, those go to version, and the rest go to the extra. Does that sound good?
There was a problem hiding this comment.
The current approach provides a clear rule that (1) if it is semver, then can leverage those additional fields, (2) if it is not semver, everything is still stored in the version string.
Okay, I guess that is fine.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5116 +/- ##
==========================================
+ Coverage 81.77% 81.80% +0.02%
==========================================
Files 340 340
Lines 140102 140237 +135
Branches 140102 140237 +135
==========================================
+ Hits 114568 114714 +146
+ Misses 21729 21716 -13
- Partials 3805 3807 +2
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:
|
jackye1995
force-pushed
the
semver-imcompat
branch
from
072576a to
29bc5b6
Compare
|
Failure due to flaky tests, merging |
…ormat#5116) WIth previous CI change, we now use the actual prerelease version as writer version. However, old version cannot parse such version string and cause panic. This PR makes sure that the version in WriterVersion is always just major.minor.patch. Any prerelease and build metadata are stored separately and not visible to old clients.
4 participants
WIth previous CI change, we now use the actual prerelease version as writer version. However, old version cannot parse such version string and cause panic.
This PR makes sure that the version in WriterVersion is always just major.minor.patch. Any prerelease and build metadata are stored separately and not visible to old clients.