-
Notifications
You must be signed in to change notification settings - Fork 407
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
calrify type of pitch constructor argument #1746
Conversation
Hi @float3 (hill) thanks for the PR -- it's nearly perfect for what we should have here given the code I'm leaning instead though to removing the line -- it comes form a type of coding paradigm in 2014 that I don't think we would do today (from before Typed python and automatic code generation). I think that we're better off getting rid of it once I see if it's used anywhere in music21 -- the whole test suite passes so I think not. So I want to thank you for the PR but I think we'll go in the opposite direction and trust the Docs and change the code to match) There's also a bug in the code for documenting Pitch as acceptable to
we did not check I think for that reason better to get rid of it. Will make creating pitches a few ms faster which is always appreciated. Will leave open for a day or two to see if there are any objections to this approach. |
I see, I've adjusted the PR, thanks for your response |
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.
Approved!
My guess is that the line was the way it was because it's called with a mixture of strings and Pitch objects.
Some mypy errors were from updating mypy and unrelated. Fixed in #1747 |
Thank you -- congrats on your first commit to music21! :-) The community will long be grateful! |
Changes unknown |
thank you and thank you for music21! hope to contribute again if I find anything else |
Line 1926 says that the argument can be a Pitch too so the constructor should reflect that