Repository: cuthbertLab/music21 (python)

Issue:
Making voices can disrupt makeAccidentals()
Based on case reported by Igor Praznik, music21 list 3/3/20.

```
s = stream.Stream()
s.insert(key.Key('Gb'))
s.insert(0, note.Note('D-5'))
s.insert(0, note.Note('D-4'))
s.show()  # During export to xml, makeNotation(), makeVoices(), makeMeasures()
```
<img width="138" alt="Screen Shot 2020-07-14 at 9 19 28 PM" src="https://user-images.githubusercontent.com/38668450/87492057-b5828a00-c617-11ea-86ca-9e4ea46d96c3.png">

`makeNotation()` calls `makeAccidentals()` per `Measure` object, but `makeAccidentals()` only inspects `measure.notesAndRests`, which is empty in this case because the notes have been put in voices already:

https://github.com/cuthbertLab/music21/blob/93bea7cc87bdb9fb2bb0ab0cb3faef1717b24004/music21/stream/__init__.py#L5867


Need to grab the voice objects somewhere, possibly just recursively calling `makeAccidentals()`, but there are going to be corner cases here with accidentals tied from previous measures.
 Update: I think this is only affecting the code path where measures have not been created yet. In that case, probably easier to just call `makeAccidentals()` before `makeVoices()` and `makeMeasures()`, since without measures to worry about, those corner cases go away.

Pull request:
Let makeAccidentals() recurse
Fixes #577. Easy patch, so I just forged on.


Before: calling `makeAccidentals()` on a Measure with substreams would have no effect. Such calls are made by `makeNotation()` and xml export.

Now: just let `makeAccidentals()` recurse over the input Stream.

Question: is there a valid use case for not recursing over substreams when making accidentals?  If so, this needs a couple quick lines of documentation in `makeAccidentals()`, possibly a keyword `recurse=`, and a decision about whether the default should be True or False in v.6 and whether it should move to True in v.7, etc.

I'm having trouble imagining the use case for NOT wanting accidentals made on substreams, so I didn't create a keyword arg. If a user doesn't want the new behavior, well, `makeAccidentals()` wasn't doing anything to substreams before, so such a user could just call `makeAccidentals()` directly on the desired substreams, if any.

`makeNotation()` now does what it advertises in more cases, so I didn't see a specific reason to update the docs, but happy to if you think wise.


Patch:
diff --git a/music21/stream/__init__.py b/music21/stream/__init__.py
index e3d28c408..2acb6b31d 100644
--- a/music21/stream/__init__.py
+++ b/music21/stream/__init__.py
@@ -5864,19 +5864,16 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
         #   alteredPitches])
 
         # need to move through notes in order
-        # NOTE: this may or may have sub-streams that are not being examined
-        noteStream = returnObj.notesAndRests
+        # recurse to capture notes in substreams: https://github.com/cuthbertLab/music21/issues/577
+        noteIterator = returnObj.recurse().notesAndRests
 
         # environLocal.printDebug(['alteredPitches', alteredPitches])
         # environLocal.printDebug(['pitchPast', pitchPast])
 
-        # # get chords, notes, and rests
-        # for i in range(len(noteStream)):
-        #     e = noteStream[i]
         if tiePitchSet is None:
             tiePitchSet = set()
 
-        for e in noteStream:
+        for e in noteIterator:
             if isinstance(e, note.Note):
                 if e.pitch.nameWithOctave in tiePitchSet:
                     lastNoteWasTied = True
@@ -6058,7 +6055,7 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
                 pass
 
         # note: this needs to be after makeBeams, as placing this before
-        # makeBeams was causing the duration's tuplet to loose its type setting
+        # makeBeams was causing the duration's tuplet to lose its type setting
         # check for tuplet brackets one measure at a time
         # this means that they will never extend beyond one measure
         for m in measureStream:
@@ -8786,6 +8783,11 @@ class Stream(core.StreamCoreMixin, base.Music21Object):
         {0.0} <music21.note.Note B>
         {2.0} <music21.chord.Chord A B->
         {3.0} <music21.note.Note D>
+
+        (Technical note: All elements of class NotRest are being found
+        right now.  This will eventually change to also filter out
+        Unpitched objects, so that all elements returned by
+        `.notes` have a `.pitches` attribute.
         '''
         if 'notes' not in self._cache or self._cache['notes'] is None:
             noteIterator = self.getElementsByClass('NotRest')
diff --git a/music21/stream/makeNotation.py b/music21/stream/makeNotation.py
index 165003b69..42e9771ee 100644
--- a/music21/stream/makeNotation.py
+++ b/music21/stream/makeNotation.py
@@ -231,7 +231,7 @@ def makeMeasures(
 
     Here is a simple example of makeMeasures:
 
-    A single measure of 4/4 is created by from a Stream
+    A single measure of 4/4 is created from a Stream
     containing only three quarter notes:
 
     >>> from music21 import articulations


Test patch:
diff --git a/music21/test/testStream.py b/music21/test/testStream.py
index 8925c2bef..4f76c7174 100644
--- a/music21/test/testStream.py
+++ b/music21/test/testStream.py
@@ -5993,13 +5993,22 @@ class Test(unittest.TestCase):
         p = stream.Part()
         p.append([m1, m2])
         # p.show()
-        # test result of xml output to make sure a natural has been hadded
+        # test result of xml output to make sure a natural has been added
         GEX = m21ToXml.GeneralObjectExporter()
         raw = GEX.parse(p).decode('utf-8')
         self.assertGreater(raw.find('<accidental>natural</accidental>'), 0)
-        # make sure original is not chagned
+        # make sure original is not changed
         self.assertFalse(p.haveAccidentalsBeenMade())
 
+    def testHaveAccidentalsBeenMadeInVoices(self):
+        s = Score()
+        s.insert(key.Key('Gb'))
+        s.insert(0, note.Note('D-5'))
+        s.insert(0, note.Note('D-4'))
+        post = s.makeNotation()  # makes voices, makes measures, makes accidentals
+        self.assertEqual(len(post.recurse().getElementsByClass('Voice')), 2)
+        self.assertTrue(post.haveAccidentalsBeenMade())
+
     def testHaveBeamsBeenMadeA(self):
         from music21 import stream
         m1 = stream.Measure()