Eliminate redundancy between Note 'dynamics' and 'channel & velocity'

Keep 'dynamics' as a read-only property, for compatibility.

Author: edudobay

mingus/containers/note.py

@@ -23,6 +23,11 @@
 from math import log
 import six
 
+_DEFAULT_NAME = "C"
+_DEFAULT_OCTAVE = 4
+_DEFAULT_CHANNEL = 1
+_DEFAULT_VELOCITY = 64
+
 
 class Note(object):
 
@@ -41,11 +46,10 @@ class Note(object):
     and chords.
     """
 
-    name = "C"
-    octave = 4
-    dynamics = {}
-    channel = 1
-    velocity = 64
+    name = _DEFAULT_NAME
+    octave = _DEFAULT_OCTAVE
+    channel = _DEFAULT_CHANNEL
+    velocity = _DEFAULT_VELOCITY
 
     def __init__(self, name="C", octave=4, dynamics=None):
         if dynamics is None:
@@ -55,21 +59,28 @@ def __init__(self, name="C", octave=4, dynamics=None):
         elif hasattr(name, "name"):
             # Hardcopy Note object
             self.set_note(name.name, name.octave, name.dynamics)
-            if hasattr(name, "channel"):
-                self.channel = name.channel
-            if hasattr(name, "velocity"):
-                self.velocity = name.velocity
         elif isinstance(name, int):
             self.from_int(name)
         else:
             raise NoteFormatError(
                 "Don't know what to do with name object: " "'%s'" % name
             )
 
+    @property
+    def dynamics(self):
+        return {
+            "channel": self.channel,
+            "velocity": self.velocity,
+        }
+
     def set_channel(self, channel):
+        if not 0 <= channel < 16:
+            raise ValueError("MIDI channel must be 0-15")
         self.channel = channel
 
     def set_velocity(self, velocity):
+        if not 0 <= velocity < 128:
+            raise ValueError("MIDI velocity must be 0-127")
         self.velocity = velocity
 
     def set_note(self, name="C", octave=4, dynamics=None):
@@ -80,23 +91,27 @@ def set_note(self, name="C", octave=4, dynamics=None):
         """
         if dynamics is None:
             dynamics = {}
+        if "channel" in dynamics:
+            self.set_channel(dynamics["channel"])
+        if "velocity" in dynamics:
+            self.set_velocity(dynamics["velocity"])
+
         dash_index = name.split("-")
         if len(dash_index) == 1:
             if notes.is_valid_note(name):
                 self.name = name
                 self.octave = octave
-                self.dynamics = dynamics
                 return self
             else:
                 raise NoteFormatError(
                     "The string '%s' is not a valid "
                     "representation of a note in mingus" % name
                 )
         elif len(dash_index) == 2:
-            if notes.is_valid_note(dash_index[0]):
-                self.name = dash_index[0]
-                self.octave = int(dash_index[1])
-                self.dynamics = dynamics
+            note, octave = dash_index
+            if notes.is_valid_note(note):
+                self.name = note
+                self.octave = int(octave)
                 return self
             else:
                 raise NoteFormatError(
@@ -107,9 +122,12 @@ def set_note(self, name="C", octave=4, dynamics=None):
 
     def empty(self):
         """Remove the data in the instance."""
+        # TODO: Review these two. This seems to leave the object in an invalid state
         self.name = ""
-        octave = 0
-        dynamics = {}
+        self.octave = 0
+
+        self.channel = _DEFAULT_CHANNEL
+        self.velocity = _DEFAULT_VELOCITY
 
     def augment(self):
         """Call notes.augment with this note as argument."""

mingus/midi/midi_track.py

@@ -61,17 +61,8 @@ def play_Note(self, note):
         To set the channel on which to play this note, set Note.channel, the
         same goes for Note.velocity.
         """
-        velocity = 64
-        channel = 1
-        if hasattr(note, "dynamics"):
-            if "velocity" in note.dynamics:
-                velocity = note.dynamics["velocity"]
-            if "channel" in note.dynamics:
-                channel = note.dynamics["channel"]
-        if hasattr(note, "channel"):
-            channel = note.channel
-        if hasattr(note, "velocity"):
-            velocity = note.velocity
+        channel = note.channel
+        velocity = note.velocity
         if self.change_instrument:
             self.set_instrument(channel, self.instrument)
             self.change_instrument = False
@@ -130,17 +121,8 @@ def play_Track(self, track):
 
     def stop_Note(self, note):
         """Add a note_off event for note to event_track."""
-        velocity = 64
-        channel = 1
-        if hasattr(note, "dynamics"):
-            if "velocity" in note.dynamics:
-                velocity = note.dynamics["velocity"]
-            if "channel" in note.dynamics:
-                channel = note.dynamics["channel"]
-        if hasattr(note, "channel"):
-            channel = note.channel
-        if hasattr(note, "velocity"):
-            velocity = note.velocity
+        channel = note.channel
+        velocity = note.velocity
         self.track_data += self.note_off(channel, int(note) + 12, velocity)
 
     def stop_NoteContainer(self, notecontainer):

tests/unit/containers/test_note.py

@@ -112,3 +112,25 @@ def test_from_shorthand(self):
         self.assertTrue(Note().from_shorthand("c") == Note("C-3"))
         self.assertTrue(Note().from_shorthand("c'") == Note("C-4"))
         self.assertTrue(Note().from_shorthand("c''''''") == Note("C-9"))
+
+    def test_velocity(self):
+        self.assertEqual(42, Note("A", 4, {"velocity": 42}).velocity)
+        self.assertEqual(64, Note("A", 4, {"channel": 2}).velocity)  # default velocity
+        self.assertEqual(42, Note(Note("A", 4, {"velocity": 42})).velocity)
+
+    def test_channel(self):
+        self.assertEqual(1, Note("A", 4, {"velocity": 100}).channel)  # default channel
+        self.assertEqual(4, Note("A", 4, {"channel": 4}).channel)
+        self.assertEqual(4, Note(Note("A", 4, {"channel": 4})).channel)
+
+    def test_invalid_channel(self):
+        with self.assertRaises(ValueError):
+            Note("A", 4, {"channel": 100})
+        with self.assertRaises(ValueError):
+            Note("A", 4, {"channel": -1})
+
+    def test_invalid_velocity(self):
+        with self.assertRaises(ValueError):
+            Note("A", 4, {"velocity": 200})
+        with self.assertRaises(ValueError):
+            Note("A", 4, {"velocity": -1})