search

prashnts / pybloomfiltermmap3

Fast Python Bloom Filter using Mmap
https://github.com/prashnts/pybloomfiltermmap3
MIT License
130 stars 24 forks source link

Fix copy_template method for python3 use #10

Closed gonzalezzfelipe closed 5 years ago

gonzalezzfelipe commented 5 years ago

The copy_template method could not be used because the filename variable passed to the copy_template method was a string, not bytes as expected. This fixes that. I ran the tests, succesfully. I dont think this one line change merits a new test, but if you do let me know and I'll add one

prashnts commented 5 years ago

I think it'd be good to do a simple test which writes to disk, and then read it -- reason being, this part has been broken for far too long (#3). Would you mind adding simple test in a new file (eg. tests/usage.py) with cases for read from disk and write to disk?

If you look at tests/simpletest.py, it does use files but uses NamedTemporaryFile to do that. This probably creates enough difference from the "usual cases" that tests passed.

gonzalezzfelipe commented 5 years ago

@prashnts No problem, there I wrote a test that makes a template and checks that it is comparable with the "original" one, making the union and intersection methods available. On the other hand, I didnt really understand the read from disk and write from disk test cases that you are mentioning. As far as I understand, the read from disk is tested on the tests.simpletest.SimpleTestCase.test_open method, where a BloomFilter is created by opening a already existing one in disk, and write from disk is constantly being tested as the bloom filters are synced. Is there something that I'm missing?

prashnts commented 5 years ago

What I meant was: since tests use NamedTemporaryFile, which has some platform dependent behaviour, we might have missed edge cases. However after reviewing in more details, I recall that I'd wondered this case long time ago and came to conclusion that it'd be unnecessary -- the platform dependent part doesn't change anything from the lib's abstraction level.

I'll merge this now, thanks!

prashnts commented 5 years ago

I had forgotten to make a new release, which I just did. v0.4.19 is up on PyPI. @gonzalezzfelipe :)