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

Supporting for Python 2 and 3 with the same code base #1240

Merged
merged 17 commits into from
Nov 14, 2014

Conversation

euphoris
Copy link
Contributor

resolve #948

@goodfeli
Copy link
Contributor

If the tests would fail on the intermediate commits, please squash them before the merge. That way we don't need to use "git bisect --skip" as often.

@euphoris
Copy link
Contributor Author

@goodfeli Tests failed on 5 commits. Because most of fixes are simple syntactic changes (e.g. from except Excetion, e to except Exception as e), any functional test wasn't failed. Failed tests were only related to pep8 (too long line and docstring). Do I need to squash these commits?

@goodfeli
Copy link
Contributor

No, if it's only test_format.py that fails, it's fine not to squash.

@goodfeli
Copy link
Contributor

I'm fine with merging this, do we need approval from a current CCW member?

@bartvm
Copy link
Contributor

bartvm commented Nov 14, 2014

I skimmed through this PR this morning and it looked fine to me. We might need to update the pull request checklist in the future to reflect the changes to using e.g. xrange.

@vdumoulin
Copy link
Member

I'm also fine with it being merged.

Vincent Dumoulin, B.Sc. Physique et Informatique
Étudiant au Ph.D.
LISA, Université de Montréal
Bureau: 3336, pavillon André-Aisenstadt
vincent.dumoulin@umontreal.ca

On Fri, Nov 14, 2014 at 4:04 PM, Bart van Merriënboer <
notifications@github.com> wrote:

I skimmed through this PR this morning and it looked fine to me. We might
need to update the pull request checklist in the future to reflect the
changes to using e.g. xrange.


Reply to this email directly or view it on GitHub
#1240 (comment).

@vdumoulin
Copy link
Member

It probably will mean that all current pull requests will need to be rebased, though.

@goodfeli
Copy link
Contributor

I think it's better to rebase the other pull requests than to rebase this one. This one is huge and very hard to rebase. It is also more vulnerable to needing additional rebases since anything we merge is likely to conflict with it. So I'm going to merge it now to avoid this PR getting stuck in an infinite rebase loop.

goodfeli pushed a commit that referenced this pull request Nov 14, 2014
Supporting for Python 2 and 3 with the same code base
@goodfeli goodfeli merged commit aabde40 into lisa-lab:master Nov 14, 2014
@dwf
Copy link
Contributor

dwf commented Nov 14, 2014

Erm, I wish I had seen this earlier. This introduces a dependency on six. Are we okay with this?

I'd prefer just throwing six in packaged_dependencies personally.

@lamblin
Copy link
Member

lamblin commented Nov 14, 2014

six is packaged with Theano, so I guess we could use that version.

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.

Python 3 support
6 participants