[MRG+2] ENH/MAINT Check for changes in pxd files too and cleanup the code #6254
Conversation
| rc = subprocess.call(['cython'] + | ||
| flags + ["-o", gen_file, cython_file]) | ||
| if rc != 0: | ||
| raise Exception('Cythonizing failed') |
There was a problem hiding this comment.
Could you please include the name of the cython file in the error message?
|
LGTM. |
35ef86a to
bc81136
Compare
|
Done. Thanks for the review! Should the 2 commits be squashed? They do 2 different things. |
|
Lets keep them separated. |
|
@arthurmensch Could you take a look at this? :) |
|
Wait there is yet another cython file format ".pxi" |
|
Hmm checking that cannot be done cleanly :@ We need to check if a Or maybe raise a warning saying that the file contains pxi files whose changes might not be tracked. And to recompile add some some whitespace changes to this pyx file if they have made any changes in the included pxi file. |
|
Currently BTW what's wrong with simply putting it in a proper (pyx + pxd) file (sorry if I am dumb, but isn't this actually better than including the code at both the places) |
bc81136 to
b93ccb9
Compare
|
Neat, the code needed some cleanup. Use of .pxi files is kind of exotic I reckon (in fact you can always avoid them using a .pxd file), I would be surprised to see new pxi files added to scikit-learn so I would think special casing |
|
Thanks for the review!! Let us merge this and raise an issue for |
[MRG+2] ENH/MAINT Check for changes in pxd files too. Cleanup code2
|
Merged, please open an issue for |
The cython code should compile when there is a change in the corresponding pxd file too.
Also cleaned up the variable names and simplified a bit.
Since I am using a
try...exceptblock, this should work even with an oldcythonize.datPlease review - @arthurmensch @amueller @ogrisel @MechCoder