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

Add Python3 support #84

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

jcfr
Copy link

@jcfr jcfr commented Apr 17, 2019

These changes are expected to be compatible with Python 2 and can be integrated.

Note that more work is required to fully support Python 3

jcfr added 5 commits April 17, 2019 00:59
This code  was originally integrated in 5b9fc95 (Initial commit of
Slicer4 module code)
This commit was crafted following these steps:

1. create and activate virtual environment

2. install the "future" package

  pip install future

3. apply the "libfuturize.fixes.fix_absolute_import" fix

  for f in `find ./ -name "*.py"`; do \
    futurize -f libfuturize.fixes.fix_print_with_import --nobackups --write $f; \
  done
This commit was crafted following these steps:

1. create and activate virtual environment

2. install the "future" package

  pip install future

3. apply the "libpasteurize.fixes.fix_newstyle" fix

  for f in `find ./ -name "*.py"`; do \
    futurize -f libpasteurize.fixes.fix_newstyle --nobackups --write $f; \
  done

4. revert addition of "from builtins import object"

  for f in `find ./ -name "*.py"`; do \
    sed -i '/from builtins import object/ d' $f; \
  done
This commit was crafted following these steps:

1. create and activate virtual environment

2. install the "future" package

  pip install future

3. apply the "libfuturize.fixes.fix_raise" and "lib2to3.fixes.fix_except" fixes

  for f in `find ./ -name "*.py"`; do \
    futurize -f libfuturize.fixes.fix_raise -f lib2to3.fixes.fix_except --nobackups --write $f; \
  done
This commit was crafted following these steps:

1. create and activate virtual environment

2. install the "future" package

  pip install future

3. apply the "libfuturize.fixes.fix_absolute_import" fix

  for f in `find ./ -name "*.py"`; do \
    futurize -f libfuturize.fixes.fix_absolute_import --nobackups --write $f; \
  done
@jcfr jcfr changed the title Add Python3 support [WIP] Add Python3 support Apr 17, 2019
@jcfr jcfr requested a review from ljod April 17, 2019 14:12
@ljod
Copy link
Member

ljod commented Apr 17, 2019

Thank you JC this is amazing.

@jhlegarreta
Copy link
Contributor

@jcfr thanks for doing this. Looks like many of the changes in ee7abcb and the change in e3238a9 have been merged through other PRs, e.g. #118.

A few notes:

  • Python 2 reached EOL a while a go, so maintaining Python 2 compatibility does not look like it is worthwhile now.
  • Some commits (e.g. 2ea70c4, ed13549) seem not to be in line with modern, Python 3 practices.
  • Using a dot is still a relative import (i.e. 2ea70c4), right? Slightly related, going forward, we should import the modules needed, and not the whole whitematternaalysis.

Do you want to rebase this on master or drop some commits and see what remains to be improved or do you think the PR should be closed?

@jhlegarreta
Copy link
Contributor

Notes after investigating further the changes:

  • BUG: Fix syntax of Slicer4/TractCluster/test.py e3238a9 : not integrated. The module is gone. Needs investigation why.
  • STYLE: Update python scripts to use print function for python 3.x ee7abcb :
    • It is unfortunate that tests and testing data were removed in commit 7c71fb1. We need to recover those (at least all parts that still apply).
    • Print statements were modernized in commit da3d285, PR 2to3 #90.
  • STYLE: Update python classes to follow new-style ed13549 : not integrated. Subclassing object is unnecessary since Python 3.x. Can drop this commit.
  • STYLE: Update use of except and raise to work with Python 3.x 561748a : the modified two files are gone. Needs investigation why 7c71fb1 removed them.
  • STYLE: Update python scripts to use absolute import for python 3.x 2ea70c4 : wma relative (because these are relative imports, not absolute) imports were adopted in da3d285, PR 2to3 #90. Can drop this commit.

@jcfr
Copy link
Author

jcfr commented Oct 6, 2023

Thanks for working on integrating.

@jcfr
Copy link
Author

jcfr commented Oct 6, 2023

Do not hesitate to rebase the pull-request, I will not have to bandwidth to work on this.

@jhlegarreta
Copy link
Contributor

Thanks @jcfr. Will work on this as time permits (long-term effort). Thanks for your past contributions to the tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants