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.

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

2 0 for real #137

Merged
merged 6 commits into from
Jul 4, 2024
Merged

2 0 for real #137

merged 6 commits into from
Jul 4, 2024

Conversation

Dr-Emann
Copy link
Member

@Dr-Emann Dr-Emann commented Jul 1, 2024

Add some tests which will fail until we get a 4.0.1 of CRoaring. Don't merge until we update again once CRoaring is fixed.

Also, include the matching CRoaring version in the readme and some clarification on versioning, this will hopefully help clarify somewhat the questions raised in #136. Link to the rendered added section: https://github.com/Dr-Emann/croaring-rs/blob/2_0_for_real/README.md#croaring-version

Fixes #136

@Dr-Emann
Copy link
Member Author

Dr-Emann commented Jul 1, 2024

@saulius, what are your thoughts on matching the CRoaring versioning exactly (bumping croaring-sys to 4.0.0) for croaring-sys, at least the major versions could probably match, any major version bump for them is almost guaranteed to be a major version bump for it, and I don't foresee any reason we'd make breaking changes to the sys crate other than updating to a new CRoaring version.

@saulius
Copy link
Member

saulius commented Jul 1, 2024

@Dr-Emann I'd agree for the croaring-sys to match the version. For the croaring crate I'd say no, as it implies we should have feature parity with CRoaring which I guess we cannot commit to. Also if there's a bug in Rust code, we'll have to break the version pin to release a bugfix.

@Dr-Emann
Copy link
Member Author

Dr-Emann commented Jul 1, 2024

Yeah, absolutely it doesn't make sense to match on the croaring crate itself.

@Dr-Emann Dr-Emann marked this pull request as ready for review July 3, 2024 21:52
@Dr-Emann Dr-Emann marked this pull request as draft July 3, 2024 23:39
Adding must_use is technically a breaking change, would like to get this in now
@Dr-Emann Dr-Emann marked this pull request as ready for review July 3, 2024 23:48
@Dr-Emann
Copy link
Member Author

Dr-Emann commented Jul 3, 2024

Also noticed some functions which should have must_use were missing it, so I went through and checked every function.

@Dr-Emann Dr-Emann merged commit 0d3ff0c into RoaringBitmap:master Jul 4, 2024
18 checks passed
@Dr-Emann Dr-Emann deleted the 2_0_for_real branch July 4, 2024 03:21
@saulius
Copy link
Member

saulius commented Jul 4, 2024

Great work @Dr-Emann, thanks for shipping it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

croaring 2.0 yanked?
2 participants