Conversation
Those properties are within the classes and don't need space in the instances. Py3k complaints about them as the properies shadow the slots.
This gets rid of all those annoying ord() calls Use next() instead of .next() Use b"" where comparing with the raw data
Minor reformat
Ask for read() and write() attribute to avoid check for unicode which does not exist in Python3.
May need some love
|
The metaclass patch uses the "future" module. This is what trips up the test suite. If adding the external dependency is unacceptable it is possible just just copy the function in or write something similar. |
|
Try to build again with this pull request, and works great with python3 without this error, ever again: Applaud 👏 |
mgedmin
left a comment
mgedmin
left a comment
There was a problem hiding this comment.
A drive-by code review.
I've ported a number of projects to Python 3 over the years, and I like what I see here. Only a couple of small problems that should be easy to fix.
| def __lt__(self, other): | ||
| if self.tick < other.tick: | ||
| return True | ||
| return self.data < other.data |
There was a problem hiding this comment.
You should be defining all six rich comparison operators, or using the functools.total_ordering class decorator to get them.
| def __lt__(self, other): | ||
| return (super(Event, self).__lt__(other) or | ||
| (super(Event, self).__eq__(other) and | ||
| self.channel < other.channel)) |
There was a problem hiding this comment.
This changes the logic instead of just porting to Python 3. Intentional?
(Also, see above re: functools.total_ordering)
| self.RunningStatus = None | ||
| trksz = self.parse_track_header(midifile) | ||
| trackdata = iter(midifile.read(trksz)) | ||
| trackdata = iter(bytearray(midifile.read(trksz))) |
| self.RunningStatus = None | ||
| for event in track: | ||
| buf += self.encode_midi_event(event) | ||
| buf = self.encode_track_header(len(buf)) + buf |
There was a problem hiding this comment.
This applies to the original code too: calling += in a loop might lead to O(N**2) running time. It would be more efficient to collect event representations into a list and combine them at the end with b''.join().
| self.window_edge = ticksleft + self.tempo.tick | ||
|
|
||
| def next(self): | ||
| def __next__(self): |
There was a problem hiding this comment.
You need to keep a next alias for Python 2! E.g.
def next(self):
...
__next__ = next| return res | ||
|
|
||
| result.append(value) | ||
| return result |
| @@ -1,4 +1,5 @@ | |||
| import math | |||
| from future.utils import with_metaclass | |||
There was a problem hiding this comment.
I personally would be inclined to use six instead of future -- it seems less intrusive. Although the actual with_metaclass implementation is just 8 lines of code, so maybe just copy it here to avoid dealing with extra dependencies?
|
I've attempted to complete this work in #130. |
4 participants
As promised in #35 here a set of patches that make the master branch (more) compatible with Python 3. As I am not using the sequencer there may still be issues with Python3 left. In general the changes are supposed to keep the code working with Python >=2.6 while also making it run with Python 3.
This patch set does not port all changes that have been made to the features/Python3 branch.
This patch set breaks the compatibility of the functions in src/util.py basically assuming they are private.