visualfabriq / bquery

A query and aggregation framework for Bcolz (W2013-01)
https://www.visualfabriq.com
BSD 3-Clause "New" or "Revised" License
56 stars 11 forks source link

Jinja #16

Closed FrancescElies closed 9 years ago

FrancescElies commented 9 years ago

Explored an idea @esc proposed in https://groups.google.com/forum/#!msg/bcolz/kGjWKVuQG8s/NZ6IQz85qvgJ using jinja templates for producing cython code. This would help maintenance

CarstVaartjes commented 9 years ago

Sounds great; so we need to do this for all the functions right + need to document a bit how this works in term of code generation etc. (I can imagine that it can make debugging quite harder)

esc commented 9 years ago

I just had a glance and this looks exactly like what I was thinking. Using the control statements of Jinja too! I like!

As for the debugging, this may make it easier to debug, since we can generate the cython sources and they actually remain legible. I should image that this might help. It's certainly going to make it easier to debug compared to using raw C and macros for metaprogramming(if that is even the right term). Not yet sure how one would integrate this build step into the setup.py but that is somewhat secondary right now.

Also note that this is a very uncommon or novel approach -- at least I haven't heard of anyone doing it like this. As such we may encounter criticism and potentially pitfalls that we have not anticipated.

FrancescElies commented 9 years ago

rebased on top of last changes on master

esc commented 9 years ago

All hail the rebase!

ARF1 commented 9 years ago

@FrancescElies I really love this: the code duplication was making my head hurt.

It was keeping me from making some improvements (e.g. direct indexing into arrays) because I was dreading adapting all the different versions of the same code and I feared accidentally breaking things because I overlooked some subtle difference.

The code generation template makes this problem really simple. It allows spotting the differences at a single glance!

@CarstVaartjes I could imagine it makes debugging easier: one could still debug the pyx file generated from the template as usual. One can even try out fixes in the pyx. - One just needs to put the final fix into the template: with the upside that one cannot forget one of the duplicated code segments.

@esc I adapted setup.py to build the extension from the templates. (See: FrancescElies/bquery#1) Instead of python setup.py build_ext one can now simply use python setup.py build_ext --from-templates to automatically regenerate the pyx from the template before distutils continues to build the extension.

FrancescElies commented 9 years ago

@ARF1 Very glad you like it, jinja templates was @esc's idea only thing I did was, giving it a try, I also struggled with code duplication while doing modifications and facing the same fears, hopefully this will help making our life a bit easier.

I merged your PR on the other branch and I cherry picked your setup.py modification for this branch too, hopefully the rest of the people are ok with that. Maybe you have already seen, in the last days we gave travis-ci the proper permissions to access this repository, therefore on new pushes we should be able to see if tests pass or not, it is a small test suite.

I am sure tests could be improved and some of them might be not very readable, but at least will give us a sort of feeling if modifications are still good. Though I am not sure if they will be enough to test racing conditions once we introduce multi-threaded code.

FrancescElies commented 9 years ago

Carst is also fine with this, therefore if you @esc & @ARF1 have no objection I think we are ready to merge.

ARF1 commented 9 years ago

I am all for it.

esc commented 9 years ago

Please do, I'm excited about how this approach will turn out! It looks promising!

FrancescElies commented 9 years ago

merging