search

pytries / marisa-trie

Static memory-efficient Trie-like structures for Python based on marisa-trie C++ library.
https://marisa-trie.readthedocs.io/en/latest/
MIT License
1.05k stars 92 forks source link

add has_keys_with_prefix method #7

Closed hickford closed 11 years ago

hickford commented 11 years ago

Fix for https://github.com/kmike/marisa-trie/issues/6

I've never written C/C++ extensions for Python. Do I need to change those files too?

kmike commented 11 years ago

Hey Matt,

Yes, you should change those files. marisa-trie uses Cython for generating actual C/C++ Python extension files - you shouldn't change these files manually. Install Cython (pip install -U cython) and run "update_cpp.sh" script to recreate those files with your changes applied. I've done a bad job describing this here: https://github.com/kmike/marisa-trie#how-is-source-code-organized

It is better to create at least two commits: for changes in pyx/pxd files (just as you've done) and for auto-generated changes in C++ files because C++ changes can be huge and unmeaningful. These generated files are stored in VCS to remove install-time dependency on Cython.

Your changes looks good, but there are a couple of comments.

  1. I think that has_keys_with_prefix method should be added to _Trie class, not to Trie, because BytesTrie and RecordTrie won't have this method otherwise. It is quite subtle why this method will work as-is for BytesTrie and RecordTrie, so it is a good idea to add a test for that as well.
  2. This is not necessary, but a benchmark for this new method could be interesting :)
  3. Could you please add yourself to "Authors & Contributors" section in README?
hickford commented 11 years ago

Hi Mikhail. Thanks for your instructions, this looks fun.

kmike commented 11 years ago

Thanks! A PR with README change is welcome :)