Skip to content
This repository was archived by the owner on Jun 19, 2025. It is now read-only.

Jinja#16

Merged
FrancescElies merged 11 commits into
visualfabriq:masterfrom
FrancescElies:jinja
Mar 13, 2015
Merged

Jinja#16
FrancescElies merged 11 commits into
visualfabriq:masterfrom
FrancescElies:jinja

Conversation

@FrancescElies

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

esc commented Mar 9, 2015

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

rebased on top of last changes on master

@esc

esc commented Mar 10, 2015

Copy link
Copy Markdown
Contributor

All hail the rebase!

@FrancescElies FrancescElies mentioned this pull request Mar 10, 2015
@ARF1

ARF1 commented Mar 12, 2015

Copy link
Copy Markdown

@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#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.

ARF and others added 2 commits March 12, 2015 14:56
Use: setup.py build_ext --from-templates
@FrancescElies

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Contributor Author

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

@ARF1

ARF1 commented Mar 13, 2015 via email

Copy link
Copy Markdown

@esc

esc commented Mar 13, 2015

Copy link
Copy Markdown
Contributor

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

@FrancescElies

Copy link
Copy Markdown
Contributor Author

merging

FrancescElies added a commit that referenced this pull request Mar 13, 2015
@FrancescElies FrancescElies merged commit 664b9c6 into visualfabriq:master Mar 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants