-
Notifications
You must be signed in to change notification settings - Fork 434
June miscellany #1034
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
June miscellany #1034
Conversation
22c1d16 to
2e86e2a
Compare
2e86e2a to
b54badc
Compare
mscuthbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! One little change requested -- then feel free to merge.
| "kernelspec": { | ||
| "display_name": "Python 3", | ||
| "language": "python", | ||
| "display_name": "Python 3.10.0b1 64-bit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh -- good to know that music21 is working on 3.10-beta. I hadn't tested it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the only pain point is some enum repr()s have to be changed in doctests. more later...
| import re | ||
| import os | ||
| import unittest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really shocked that this didn't cause a pylint error...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have it turned off because we have wildcard imports. And speaking of which, that must have been how identified these, I was looking at pylint reports when poking at #881 (which BTW I think I have a solution for)
| >>> r2 = note.Rest(quarterLength=5.0) | ||
| >>> r2.fullMeasure = True | ||
| >>> m2 = stream.Measure(r2) | ||
| >>> m2.insert(0, meter.TimeSignature('6/4')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that 5/4 would make this clearer, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a comment to explain, clearly. This is showing that when the rest doesn't fill the measure but we set .fullMeasure = True anyhow, we still treat as full measure and don't split it.
|
We haven't really discussed our policy on squashing vs. rebasing/merge commits. I think this one's too eclectic to squash. It also has the benefit of letting you see the last three commits I pushed. |
Highlights are removing the "mx" file format and adding some test coverage for the
splitAtDurations()edits in #1005.