Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] Cython syntax error #1730

Merged
merged 1 commit into from Feb 13, 2019

Conversation

@skoudoro
Copy link
Member

commented Feb 4, 2019

This PR should fix the "PRE" Travis build error as you can see here

@arokem

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

Looks good, of course. Do you understand why we missed this until now?

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

The default language_level for Cython was python2. It seems they changed this for the future release.

@arokem

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

Gotcha. That makes sense! Thanks for fixing this!

@arokem

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

I'll wait for Travis to finish, just in case anything comes up and then merge, since this is fairly uncontroversial. We're going to run into more and more of this kind of stuff, now that the community is moving away from Python 2. We should probably make a plan to transition away from Python 2 support too. I'll open an issue to discuss.

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

We should probably make a plan to transition away from Python 2 support too. I'll open an issue to discuss.

Totally agree with that point. Python 2.7 End of life in 10 month!

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

It seems that this PR fix the syntax error, but a lot of new Cython errors appears on PRE. Theses errors deserve theirs owns PR's, I think.

@codecov-io

This comment has been minimized.

Copy link

commented Feb 5, 2019

Codecov Report

Merging #1730 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1730   +/-   ##
=======================================
  Coverage   84.27%   84.27%           
=======================================
  Files         115      115           
  Lines       13606    13606           
  Branches     2144     2144           
=======================================
  Hits        11466    11466           
  Misses       1643     1643           
  Partials      497      497
@arokem

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Hi Serge - sorry for the slow reply. Do you want me to merge this, or wait for the fix on PRE? I am happy either way.

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

No worries Ariel, I am not in a hurry. Yes, you can go ahead to merge this, I will create a new PR when I understand why the new Cython version does not find all *.pxd files

@arokem arokem merged commit 818658b into nipy:master Feb 13, 2019

4 checks passed

codecov/patch Coverage not affected when comparing 9606c64...3bd7c97
Details
codecov/project 84.27% remains the same compared to 9606c64
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skoudoro skoudoro deleted the skoudoro:fix-cython-pre branch Feb 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.