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

Change bits order and fix initialize with custom field #9

Merged
merged 6 commits into from Sep 13, 2018

Conversation

Projects
None yet
2 participants
@dalibor
Copy link
Contributor

dalibor commented Jul 19, 2018

Hi @peterc ,

Would you accept a non backward compatible change for the order of the bits so that it's compliant with Redis bit operations (setbit/getbit)? Alternatively, I could add an option on initialize to support, but I don't want to complicate the code and I think that with a proper version upgrade and maybe notification of some kind this would is a better default because the bits on disk are stored in the same way and we don't have to call reverse.

What do you think?

The use case I have is initializing a bloom filter from a Redis field value to update it in memory.

Thanks!

@peterc

This comment has been minimized.

Copy link
Owner

peterc commented Jul 19, 2018

I'm a bit snowed under right now to check it out, but that's potentially such a breaking change we'd need to bump to 2.0. Also, there's a mismatch in the current version number in the two places it's defined. If we go to 2.0 so that any people depending on 1.x don't get burned, this should be OK.

@dalibor

This comment has been minimized.

Copy link
Contributor Author

dalibor commented Jul 19, 2018

@peterc Thanks for your feedback. I've bumped the version to 2.0.0. I noticed there is also the bitarray-array.rb which becomes outdated. I'm wondering whether it's worth to update it to the new format and add tests for it or we should just remove it? Any thoughts? Thanks!

@dalibor

This comment has been minimized.

Copy link
Contributor Author

dalibor commented Sep 6, 2018

@peterc I noticed some recent movement in master and updated my branch. Any thoughts about merging this PR and removing the outdated bitarray-array.rb?

Since this is not a backward-compatible change, I wonder if we should maybe provide an initializing option, i.e. change the signature to something like BitArray#initialize(size, field: nil, reverse: true) where we keep the default behaviour as it is?

@peterc

This comment has been minimized.

Copy link
Owner

peterc commented Sep 10, 2018

Yeah, I just wasn't won over by the motivation for doing this, particularly as a breaking change.

If, however, we can do it as a non-breaking change that's optional and you can ensure the tests and README update are all there, I'd have to problem with merging it in to provide more options for peole.

@dalibor

This comment has been minimized.

Copy link
Contributor Author

dalibor commented Sep 12, 2018

@peterc How do you feel about this change? Note that the bitarray-array becomes slightly outdated because it does not support this option and the new tests does not work for it (because they're string based), so maybe we should make this a 2.0 release instead of 1.2 and remove bitarray-array?

Btw, I have a new blog post that describes the use-case I have and the need for this change: https://godaddy.github.io/2018/09/11/redis-ruby-bloom-filter/.

@peterc

This comment has been minimized.

Copy link
Owner

peterc commented Sep 13, 2018

I saw the post and included it in Ruby Weekly this week, I believe, cool stuff :)

Took a quick look at your new submission and on the surface of it, it looks fine. I need to actually pull it down and test it properly, but all being well, I think we'll accept this.

@peterc peterc merged commit 12ed0ba into peterc:master Sep 13, 2018

@peterc

This comment has been minimized.

Copy link
Owner

peterc commented Sep 13, 2018

Seems fine, am merging now and will sort out a release next as there are a few minor tweaks I want to make first.

@peterc

This comment has been minimized.

Copy link
Owner

peterc commented Sep 13, 2018

And released!

@dalibor

This comment has been minimized.

Copy link
Contributor Author

dalibor commented Sep 13, 2018

Awesome! Thank you, Peter! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment