gutomaia / pyNES

Python programming for Nintendo 8 bits
BSD 3-Clause "New" or "Revised" License
1.21k stars 109 forks source link

Support Python 3 #7

Closed gutomaia closed 8 years ago

gutomaia commented 10 years ago

Support for Python 3 is a must have, especially if you intent to use pyNES in a Raspberry PI. It came with Python 3 as a standard!

ellisonleao commented 10 years ago

Sure, this is sure a nice challenge. :smile:

gutomaia commented 10 years ago

It's yours!!! :+1:

ellisonleao commented 10 years ago

Can we use the Tox project for testing multiple python versions?

gutomaia commented 10 years ago

I've never heard about Tox. I think we can, since we create a specific task on github for it. A mean, it should not interfere with the default "test" task.

But that is a nice idea. I will create minor tasks on the comments. Don't forget to fork and create a branch with a comment #7 hashtag, and also, use the same hash on each commit message.

thanks

ellisonleao commented 10 years ago

Sure! Thanks!

gutomaia commented 10 years ago

Amazing job

However, I've might forgot to tell you. You should start from the branch 0.0.x (https://github.com/ellisonleao/pyNES/network), you started from the master branch. Sorry, my bad.

Anyway. You can still merge stuff

ellisonleao commented 10 years ago

I've created a py3 branch from the master to make the changes. I'll merge the 0.0.x on the my local py3 branch and keep going, ok?

gutomaia commented 10 years ago

You're almost in the right path. There is a slight detour on the goal. See, most of the commit lines are related to PEP8, not to the main goal that is to check and make it run on Python3.

Keep track on the path, on large groups that might give several merge conflicts. Are you comfortable with that?

But, overall, it's all great! congrats!

ellisonleao commented 10 years ago

Yeah, i've started with the PEP8 check before starting the python 3 migration if that's ok with you. I am splitting the commits on regions by now (tests folder PEP8, examples folder PEP8 and so on). I am not a fan of large commits as well but i'm kepping track of current branches so the merge won't be a problem.

Thanks for the feedback!

ellisonleao commented 10 years ago

@gutomaia i am now having a problem on python 3 tests. Here's the exception:

    class NesArray(NesType, list, List):
TypeError: multiple bases have instance lay-out conflict

any thoughts on that?

danilobellini commented 10 years ago

That happens on Python 3.3 and 3.4:

>>> from ast import List
>>> class A(list, List): pass
Traceback (most recent call last):
  [...]
TypeError: multiple bases have instance lay-out conflict

But not on Python 3.2 nor 2.7. But that's about inheritance from both _ast.AST and a built-in:

>>> from ast import AST, List
>>> List.mro()
[<class '_ast.List'>, <class '_ast.expr'>, <class '_ast.AST'>, <class 'object'>]
>>> List.mro()[2] is AST
True
>>> class B(list, AST): pass
Traceback (most recent call last):
  [...]
TypeError: multiple bases have instance lay-out conflict
>>> # And something that happens in the class NesInt(int, Num, NesType):
>>> class C(int, AST): pass
Traceback (most recent call last):
  [...]
TypeError: multiple bases have instance lay-out conflict

Since Python 2.2:

You can use multiple inheritance, but you can't multiply inherit from different built-in types (for example, you can't create a type that inherits from both the built-in dict and list types). This is a permanent restriction; it would require too many changes to Python's object implementation to lift it. However, you can create mix-in classes by inheriting from "object". This is a new built-in, naming the featureless base type of all built-in types under the new system.

Probably that happens with classes from C extensions as well, classes with __slots__, etc.. This, for example, happens on both Python 2.7 and 3.x

>>> from decimal import Decimal
>>> class D(Decimal, int): pass
Traceback (most recent call last):
  [...]
TypeError: multiple bases have instance lay-out conflict
gutomaia commented 10 years ago

I knew! :dart: I saw that before the corner. I've already spoke with @ellisonleao and we both believe that figure out a way out... Good luck

ellisonleao commented 10 years ago

hey guys, long time no see. About the layout conflict, should we use some mixins classes to handle this problem or we do have a simple way to fix that? @gutomaia @danilobellini

danilobellini commented 10 years ago

As the MRO shouldn't include both AST and list, mixin classes aren't an option unless you do so in a way that gets rid from at least one of these two classes. Any solution should be beyond class inheritance. But I think it should be easier than that.

Just for fun, I removed the AST nodes from the bases in both NesArray and NesInt, as well as changing the isinstance(self.tile, List) to isinstance(self.tile, list) in the NesSprite.__len__, so that I don't even need to have an import line in that file. Yet that was enough to get all tests passing, and we know the tests includes compiling the Python files in the examples to NES assembly.

Probably @gutomaia had already prepared the room for that update, as lines like Num(NesInt(node.n)) in the composer.py are what makes this small change possible. The return List(elts=expr.elts) before the never-run return NesArray(expr.elts) line is another evidence for that.