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.

Sign up for GitHub

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

Jump to bottom

Adjust Book examples to the new deserialization API #1118

Merged
merged 1 commit into from
Nov 12, 2024
Merged

Adjust Book examples to the new deserialization API #1118

merged 1 commit into from
Nov 12, 2024

Conversation

wprzytula
Copy link
Collaborator

This PR adjusts examples in the Book to the new API, so that the tests compile and pass. This should be merged quickly to bring back the passing Book check to the CI.

Further adjustments of the Book to the new API, including more descriptive tips and a migration guide, are left as a follow-up.

Refs: #462
Depends on: #1057

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • [ ] I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • [ ] I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • [ ] I added appropriate Fixes: annotations to PR description.

@wprzytula wprzytula self-assigned this Nov 10, 2024
@wprzytula wprzytula added this to the 0.15.0 milestone Nov 10, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2024 •
edited
Loading

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: e38486b

@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Nov 10, 2024
Lorak-mmk
Lorak-mmk requested changes Nov 12, 2024
View reviewed changes
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

Looks good - all of my comments are regarding one issue: that we should use borrowed deserialization where possible.

README.md Show resolved Hide resolved
docs/source/data-types/blob.md Show resolved Hide resolved
docs/source/data-types/collections.md Show resolved Hide resolved
docs/source/data-types/collections.md Show resolved Hide resolved
docs/source/data-types/text.md Show resolved Hide resolved
docs/source/data-types/tuple.md Show resolved Hide resolved
docs/source/queries/result.md Outdated Show resolved Hide resolved
@Lorak-mmk
Copy link
Collaborator

Oh wait, most of those is probably because we have no lending stream, right?
In that case please:

  • Switch those that don't use stream to borrowed form
  • Introduce closure API and switch over the rest (or at least those that can be switched).

muzarski
muzarski suggested changes Nov 12, 2024
View reviewed changes
docs/source/queries/result.md Outdated Show resolved Hide resolved
@wprzytula
Copy link
Collaborator Author

wprzytula commented Nov 12, 2024 •
edited
Loading

Introduce closure API and switch over the rest (or at least those that can be switched).

I'm not convinced we have to introduce the closure API in this release. After all, it's going to be a new API, not breaking anything. OTOH other APIs introduced in the deserialization refactor aren't entirely new APIs; they are rather improvements over old APIs (QueryResult, TypedRowStream etc.).
What we have so far in 0.15 is a major upgrade already, reducing a lot of needless allocations (in form of Rows, Row and CqlValue). I fully support deserialization of borrowed types, but the new framework already can do that - and adding the API for that in the yet next release will be seamless for the users.

Designing the closure API, testing it, documenting it, adjusting examples etc. is going to be a large task, not suitable for 0.15 IMO.

Especially that the execution API refactor can mess with those APIs, too...

@wprzytula wprzytula force-pushed the adjust-book-to-new-deserialization-api branch from fb79108 to 4502c3a Compare November 12, 2024 14:23
@muzarski muzarski self-requested a review November 12, 2024 14:27
@wprzytula wprzytula force-pushed the adjust-book-to-new-deserialization-api branch 2 times, most recently from 5c22a8c to 4b853c5 Compare November 12, 2024 15:32
@piodul @wprzytula
book: adjust code examples to the new deserialization API
e38486b 
Examples in the Book are now adjusted to the new deserialization API,
and the book finally compile and the tests there pass.
Note that this commit does not describe exhaustively the intended way of
using the new deserialization framework; this is left for a follow-up.

Co-authored-by: Wojciech Przytuła <wojciech.przytula@scylladb.com>
@wprzytula wprzytula force-pushed the adjust-book-to-new-deserialization-api branch from 4b853c5 to e38486b Compare November 12, 2024 16:35
@github-actions github-actions bot removed the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Nov 12, 2024
@wprzytula wprzytula merged commit 814de55 into scylladb:main Nov 12, 2024
12 checks passed
@wprzytula wprzytula deleted the adjust-book-to-new-deserialization-api branch November 12, 2024 19:30
@wprzytula wprzytula mentioned this pull request Nov 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muzarski muzarski muzarski approved these changes

@Lorak-mmk Lorak-mmk Lorak-mmk approved these changes

Assignees

@wprzytula wprzytula

Labels
None yet
Projects
None yet
Milestone
0.15.0
Development

Successfully merging this pull request may close these issues.
4 participants
@wprzytula @Lorak-mmk @muzarski @piodul