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

Support running on Python 3 #3

Merged
merged 16 commits into from
Apr 22, 2016
Merged

Conversation

ncoghlan
Copy link
Contributor

This patch series covers:

  • initial single-source 2/3 support via python-modernize
  • subsequent updates to get tests to same state on both 2 & 3

@ncoghlan ncoghlan mentioned this pull request Apr 22, 2016
@bkabrda
Copy link
Contributor

bkabrda commented Apr 22, 2016

Note: when running redhawk with this patch series on Python 3, it will only work on common subset of Python 2 and Python 3. IOW, it won't work on e.g.

try:
    pass
except Exception, e: # py2 only syntax
    pass

or

async def foo(): # py3 only syntax
    pass

If run on Python 2, it will still analyze Python 2 code correctly. Analyzing Python 3 constructs when running on Python 3 will require some additional work (adding Converters and Nodes for these constructs).

@@ -130,6 +132,8 @@
import re
import sys

from six.moves import filter
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only file that the six module is now being imported for. In this file, filter is used only at Line 226. Replacing it with list(filter(..)) will enable us to remove this import, thereby removing six as a dependency altogether.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will do.

@spranesh
Copy link
Owner

I have three comments:

  • Adding a ConvertList method will clean up lots of code in the converters due to the list(map(..)) pattern
  • Dependency on six module can be removed by changing filter in one place
  • The Try node was added for Issue Support analysing new Python 3 syntactic constructs #2. All the other changes here are for Issue Python 3 #1. If you are planning on working that in the near future, it would be good to separate that out into another PR?

We will also need a way to separate out if-else pattern when Python 3 support starts. I will think about a way of doing so in the near future.

Change is looking good. Please address the above and I'll accept the Pull request. Thanks for doing this!

@bkabrda
Copy link
Contributor

bkabrda commented Apr 22, 2016

So I think I addressed all of your points, but I started testing on Python 3.5 and there seem to be some new test failures, so I'm going to investigate them. Please don't merge yet.

@bkabrda
Copy link
Contributor

bkabrda commented Apr 22, 2016

Ok, fixed Python 3.5 compatibility and pushed. Ready to be re-reviewed and merged.

@spranesh spranesh merged commit 68a7400 into spranesh:master Apr 22, 2016
@spranesh
Copy link
Owner

Changes look good - I've merged this to master. Thanks everybody for pitching in and helping fix this issue. Appreciate this! :-)

@bkabrda: Good spot with ConverListOfStatements!

@bkabrda
Copy link
Contributor

bkabrda commented Apr 23, 2016

Thanks a lot for the review and for accepting, @spranesh!

@jpopelka
Copy link

Thanks @bkabrda @spranesh @ncoghlan !

@spranesh
Copy link
Owner

Thanks @bkabrda @ncoghlan @jpopelka for your contributions! I've just released a version containing these changes (and some bug fixes). This version will enable redhawk to run via Python3.

However, it may still barf when analyzing some Python3 codebases. This is important, but will take significant development time to fix (Issue #2 and Issue #3).

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.

None yet

4 participants