Skip to content
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

Set sixthMinor and seventhMinor to CAUTIONARY in romanNumeralFromChord() #1230

Merged
merged 2 commits into from
Mar 2, 2022

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Feb 20, 2022

Fixes #1217
Fixes #1229

FYI @napulen what do you think?

Clarification: this does not change any default behavior of figure or pitch parsing, but simply sets the attributes on the created RomanNumeral object to reflect how the figure should be interpreted, given the figure it was created with. (i.e., the method creates a cautionary "b" in "bVI", so set sixthMinor to CAUTIONARY.)

@coveralls
Copy link

coveralls commented Feb 20, 2022

Coverage Status

Coverage remained the same at 93.241% when pulling 38c3a79 on figure-roundtrip into d73e6b7 on master.

@napulen
Copy link
Contributor

napulen commented Feb 21, 2022

Looks good! I have a long corpus of RomanText files, let me try it there and confirm whether the issue is fixed after this.

@napulen
Copy link
Contributor

napulen commented Feb 28, 2022

(running this corpus test later today)

@napulen
Copy link
Contributor

napulen commented Mar 1, 2022

OK, I can confirm that these default attributes of the RomanNumeral object mimic the parsing behavior of the RomanText files I have been using.

Thanks, @jacobtylerwalls!

@mscuthbert mscuthbert merged commit 3613fd0 into master Mar 2, 2022
@mscuthbert mscuthbert deleted the figure-roundtrip branch March 2, 2022 22:20
@mscuthbert
Copy link
Member

Thanks all for this improvement without introducing (much) backwards incompatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants