implemented __eq__ for MidiFile

[Nintorac]

fix for #150

[cjappl]

Please fix the tests so that they pass! To test from your own environment, you should be able to just run tox from the main directory

[carlthome]

I think this is a pretty sweet addition!

But it should be clearly documented what MidiFile objects are considered equal, because even when looking at the implementation it's not obvious what happens when the contained tracks are compared.

[Nintorac]

oh yea, my bad..for some reason I had in my head __eq__ was implemented for Track

looks like __eq__ is implemented for BaseMessage so would it be sufficient to for the track __eq__ to be something like this

def __eq__(self, other):


    for this_message, other_message in zip(sorted(self), sorted(other)):
        if this_message != other_message:
            return False
    return True

Then it would be neccesary to implement __cmp__ on BaseMessage, would it be sufficient to order base on the byte representation of a BaseMessage?

[carlthome]

__cmp__ isn't used in Python anymore, no? 😕

[belm0]

It would be better to properly implement __repr__ for Message, MidiTrack, and MidiFile. Equality comparison for MidiFile is then trivially repr(a) == repr(b). Having a proper repr() is useful for other situations like textual diff of MIDI data and automated testing. See #162.

In contrast, this PR implements a one-off comparison within __eq__. It's opaque in that users don't know what is being compared, and exactly what difference might be foiling an equal result.

[belm0]

Also, implementing only __eq__ is bad, see #163

[belm0]

#164 is merged, so repr(a) == repr(b) should be sufficient for this use case

[olemb]

It's easier than that:

• messages already have __eq__() (in BaseMessage. This compares vars(self) == vars(other), which is all we need.
• MidiTrack() is a subclass of list so it also has __eq__()
• the list of tracks is just a list so it also has __eq__

This means all we have to do is:

    def __eq__(self, other):
        if not isinstance(other, MidiFile):
            return False

        return ((self.type, self.ticks_per_beat, self.tracks)
            == (other.type, other.ticks_per_beat, other.tracks))

[olemb]

Thanks for your submission!

The filename should not be included in the comparison since it's not a part of the content of the file.

As I said in the comment there's a very simple way to implement this:

    def __eq__(self, other):
        if not isinstance(other, MidiFile):
            return False

        return ((self.type, self.ticks_per_beat, self.tracks)
            == (other.type, other.ticks_per_beat, other.tracks))

[olemb]

Ah, the review comes up here so I've repeated myself a bit. That's OK. Still learning to GitHub. :)

Thanks everyone for contributing to this discussion! I am very excited about getting this into Mido! It will make tests a lot easier.

[belm0]

I mentioned before and filed a bug, but implementing only __eq__ is problematic.

#163

[olemb]

@belm0 Now that we've merged your new repr() format you could actually just diff the repr() of each file, although it would be slower.

On the other hand, this opens up new possibilities. With this small program (repr_midifile.py):

import sys
import mido

print(repr(mido.MidiFile(sys.argv[1])))

you could do:

python repr_midifile.py file1.mid >file1.txt
python repr_midifile.py file2.mid >file2.txt
diff file1.txt file2.txt

and get a detailed diff.

Or you could do the whole thing inside Python using difflib, or you could convert the file to repr() format, edit it in a browser and recompile it with eval().

[olemb]

@belm0 Thanks for reminding me about the only implementing __eq__() thing. Let's discuss it in #163 and find a solution.

Other than that my code should work. Would you agree?

[belm0]

Other than that my code should work. Would you agree?

Actually, I didn't know that List __eq__ compares the lists element-wise.

Your proposal seems like it should work, though I don't remember the details of frozen objects. In repr I tried to keep things canonicalized:

repr is canonicalized to the actual MIDI data as much as possible. Hence MidiFile parameters like "filename", "debug" etc. are ignored, and Frozen variants are represented in mutable form.

[olemb]

Aha, I didn't catch this. I though you had made a mistake in the message repr() and changed it back.

Given this code:

import sys
import mido
import mido.frozen

mid = mido.MidiFile(tracks=[
    mido.MidiTrack([mido.frozen.FrozenMessage('note_on')])
])
print(repr(mid))

your original pull request will print:

MidiFile(type=1, ticks_per_beat=480, tracks=[
  MidiTrack([Message('note_on', time=0, channel=0, note=0, velocity=64)])
])

while my version prints:

MidiFile(type=1, ticks_per_beat=480, tracks=[
  MidiTrack([FrozenMessage('note_on', channel=0, note=0, velocity=64, time=0)])
])

I think the repr() should show the actual class of message. This would be very surprising:

>>> from mido.frozen import FrozenMessage
>>> FrozenMessage('note_on')
Message('note_on', time=0, channel=0, note=0, velocity=64)

For __eq__() we can ignore the class and return True as long as the data is identical, just like with sets and frozensets:

>>> set([1, 2, 3]) == frozenset([1, 2, 3])
True

>>> Message('note_on') == FrozenMessage('note_on')
True