-
Notifications
You must be signed in to change notification settings - Fork 54
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
release 0.4.0! #416
release 0.4.0! #416
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #416 +/- ##
=======================================
Coverage 78.82% 78.82%
=======================================
Files 51 51
Lines 10655 10655
Branches 10655 10655
=======================================
Hits 8399 8399
Misses 1788 1788
Partials 468 468 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, and very thorough, tho I don't know how to be sure it's complete?
For the breaking change list, it seems helpful to add a sentence to each item explaining what kind of breakage the change might cause for existing code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice thanks, just a few things
Thanks! And yea, this is generally done as best-effort by me going through every PR that merged since the last release..
Yea good idea, I can try to add more to breaking changes to help migrations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@@ -183,4 +183,4 @@ Some design principles which should be considered: | |||
[cargo-llvm-cov]: https://github.com/taiki-e/cargo-llvm-cov | |||
[FFI]: ffi/ | |||
[Arrow]: https://arrow.apache.org/rust/arrow/index.html | |||
[Tokio]: https://tokio.rs/ | |||
[Tokio]: https://tokio.rs/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity, what changed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the newline? idk maybe some editor junk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the file didn't have a newline at EOF before, and now it does?
f0fa784
to
c05570c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question but otherwise looks great
CHANGELOG.md
Outdated
3. new `WriterFeatures` enum variant: `TypeWidening` and `TypeWideningPreview` [\#335] | ||
4. new `Error` enum variant: `InvalidLogPath` when kernel is unable to parse the name of a log path [\#347] | ||
5. Module moved: `mod delta_kernel::transaction` -> `mod delta_kernel::actions::set_transaction` [\#386] | ||
6. output of arrow expression evaluation now applies/validates output schema in default arrow expression handler [\#331] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one really a breaking change? Seems like it would just allow previously broken stuff to no longer break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm yea I had the same question (where we should put those changes to expression evaluation). I agree it "allows previously broken stuff to no longer break" and since the semantics remain the same (just that we are enforcing/implementing it now) perhaps this goes under 'fixes'? WDYT @nicklan @scovich
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fixes is probably good. I don't know how critical it is that we get the changelog "just right" at this point, but nothing will break due to this change (i don't think) so fixes or enhancements makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! releasing!
de304a0
to
9ef83c6
Compare
Release 0.4.0 of all crates! rendered changelog link