-
-
Notifications
You must be signed in to change notification settings - Fork 31
ChordType Overhaul #47
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
Conversation
…at Five in all cases @aure
Co-authored-by: Aure <[email protected]>
Co-authored-by: Aure <[email protected]>
Co-authored-by: Aure <[email protected]>
Co-authored-by: Aurelius Prochazka <[email protected]>
Co-authored-by: Aurelius Prochazka <[email protected]>
Co-authored-by: Aure <[email protected]>
Co-authored-by: Aurelius Prochazka <[email protected]>
Co-authored-by: Aurelius Prochazka <[email protected]>
Co-authored-by: Aurelius Prochazka <[email protected]>
Co-authored-by: Aurelius Prochazka <[email protected]>
Co-authored-by: name <[email protected]>
…rity Co-authored-by: Aurelius Prochazka <[email protected]>
Co-authored-by: Aurelius Prochazka <[email protected]>
Co-authored-by: Aurelius Prochazka <[email protected]>
|
|
||
| /// B♭ Major | ||
| static var Bb = Chord(.Bb, type: .majorTriad) | ||
| static var Bb = Chord(.Bb, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Bb' (identifier_name)
|
|
||
| /// A♭ Major | ||
| static var Ab = Chord(.Ab, type: .majorTriad) | ||
| static var Ab = Chord(.Ab, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Ab' (identifier_name)
|
|
||
| /// G♭ Major | ||
| static var Gb = Chord(.Gb, type: .majorTriad) | ||
| static var Gb = Chord(.Gb, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Gb' (identifier_name)
|
|
||
| /// F♭ Major | ||
| static var Fb = Chord(.Fb, type: .majorTriad) | ||
| static var Fb = Chord(.Fb, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Fb' (identifier_name)
|
|
||
| /// E♭ Major | ||
| static var Eb = Chord(.Eb, type: .majorTriad) | ||
| static var Eb = Chord(.Eb, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Eb' (identifier_name)
|
|
||
| /// G♯ Major | ||
| static var Gs = Chord(.Gs, type: .majorTriad) | ||
| static var Gs = Chord(.Gs, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Gs' (identifier_name)
|
|
||
| /// F♯ Major | ||
| static var Fs = Chord(.Fs, type: .majorTriad) | ||
| static var Fs = Chord(.Fs, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Fs' (identifier_name)
|
|
||
| /// E♯ Major | ||
| static var Es = Chord(.Es, type: .majorTriad) | ||
| static var Es = Chord(.Es, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Es' (identifier_name)
|
|
||
| /// D♯ Major | ||
| static var Ds = Chord(.Ds, type: .majorTriad) | ||
| static var Ds = Chord(.Ds, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Ds' (identifier_name)
|
|
||
| /// C♯ Major | ||
| static var Cs = Chord(.Cs, type: .majorTriad) | ||
| static var Cs = Chord(.Cs, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Cs' (identifier_name)
|
|
||
| /// B Minor | ||
| static var Bm = Chord(.B, type: .minorTriad) | ||
| static var Bm = Chord(.B, type: .minor) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Bm' (identifier_name)
|
|
||
| /// A Minor | ||
| static var Am = Chord(.A, type: .minorTriad) | ||
| static var Am = Chord(.A, type: .minor) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Am' (identifier_name)
|
|
||
| /// G Minor | ||
| static var Gm = Chord(.G, type: .minorTriad) | ||
| static var Gm = Chord(.G, type: .minor) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Gm' (identifier_name)
|
|
||
| /// F Minor | ||
| static var Fm = Chord(.F, type: .minorTriad) | ||
| static var Fm = Chord(.F, type: .minor) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Fm' (identifier_name)
|
|
||
| /// E Minor | ||
| static var Em = Chord(.E, type: .minorTriad) | ||
| static var Em = Chord(.E, type: .minor) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Em' (identifier_name)
|
|
||
| /// G Major | ||
| static var G = Chord(.G, type: .majorTriad) | ||
| static var G = Chord(.G, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'G' (identifier_name)
|
|
||
| /// F Major | ||
| static var F = Chord(.F, type: .majorTriad) | ||
| static var F = Chord(.F, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'F' (identifier_name)
|
|
||
| /// E Major | ||
| static var E = Chord(.E, type: .majorTriad) | ||
| static var E = Chord(.E, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'E' (identifier_name)
|
|
||
| /// D Major | ||
| static var D = Chord(.D, type: .majorTriad) | ||
| static var D = Chord(.D, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'D' (identifier_name)
|
|
||
| /// C Major | ||
| static var C = Chord(.C, type: .majorTriad) | ||
| static var C = Chord(.C, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'C' (identifier_name)
|
|
||
| /// B Minor | ||
| static var Bm = Chord(.B, type: .minorTriad) | ||
| static var Bm = Chord(.B, type: .minor) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Bm' (identifier_name)
|
|
||
| /// A Minor | ||
| static var Am = Chord(.A, type: .minorTriad) | ||
| static var Am = Chord(.A, type: .minor) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Am' (identifier_name)
|
|
||
| /// G Minor | ||
| static var Gm = Chord(.G, type: .minorTriad) | ||
| static var Gm = Chord(.G, type: .minor) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Gm' (identifier_name)
|
|
||
| /// F Minor | ||
| static var Fm = Chord(.F, type: .minorTriad) | ||
| static var Fm = Chord(.F, type: .minor) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Fm' (identifier_name)
|
|
||
| /// E Minor | ||
| static var Em = Chord(.E, type: .minorTriad) | ||
| static var Em = Chord(.E, type: .minor) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'Em' (identifier_name)
|
|
||
| /// G Major | ||
| static var G = Chord(.G, type: .majorTriad) | ||
| static var G = Chord(.G, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'G' (identifier_name)
|
|
||
| /// F Major | ||
| static var F = Chord(.F, type: .majorTriad) | ||
| static var F = Chord(.F, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'F' (identifier_name)
|
|
||
| /// E Major | ||
| static var E = Chord(.E, type: .majorTriad) | ||
| static var E = Chord(.E, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'E' (identifier_name)
|
|
||
| /// D Major | ||
| static var D = Chord(.D, type: .majorTriad) | ||
| static var D = Chord(.D, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'D' (identifier_name)
|
|
||
| /// C Major | ||
| static var C = Chord(.C, type: .majorTriad) | ||
| static var C = Chord(.C, type: .major) |
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.
Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'C' (identifier_name)
|
Ayyyyyyeeee thanks for this contribution! I only had time to quickly scan this for the moment but two things I noticed:
|
|
This is obviously a great PR, but I won't accept until I make the tests pass (even if they will be incomplete). |
| public var bassNote: NoteClass { | ||
| switch inversion { | ||
| case 1...4: | ||
| case 1...type.intervals.count: |
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.
Switch and Case Statement Alignment Violation: Case statements should vertically align with their enclosing switch statement. (switch_case_alignment)
| public var bassNote: NoteClass { | ||
| switch inversion { | ||
| case 1...4: | ||
| case 1...type.intervals.count: |
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.
Switch and Case Statement Alignment Violation: Case statements should vertically align with their enclosing switch statement. (switch_case_alignment)
|
|
||
| // order the array preferring root position | ||
| returnArray.sort { $0.inversion < ($1.inversion > 0 ? 1 : 0) } | ||
|
|
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.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
|
|
||
| // prefer fewer number of characters (favor less complex chords in ranking) | ||
| returnArray.sort { $0.slashDescription.count < $1.slashDescription.count } | ||
|
|
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.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
|
@maksutovic @aure since we're taking a look at this PR I just wanted to keep applying pressure on Codable here. The raw value of our ChordType is a String. If you were to change the name of a ChordType enum case in the future then you would break the decoding of any encoding of that case that you have previously made. IMO if we are set on introducing Codable conformances, we should use some stable raw value when we do it - such as explicitly assigning each case to a specific integer value. |
aure
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.
This was a ton of work!
| /// | ||
| /// - Parameter noteSet: Array of chord notes in a chosen order | ||
| /// - Returns: array of enharmonic chords that could describe the NoteSet | ||
| @available(*, deprecated, renamed: "getRankedChords", message: "Please use getRankedChords() for higher quality chord detection") |
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.
Line Length Violation: Line should be 120 characters or less: currently 133 characters (line_length)
|
|
||
| lazy var chords: [Int: Chord] = ChordTable.generateAllChords() | ||
|
|
||
| @available(*, deprecated, renamed: "getRankedChords()", message: "Please use getRankedChords() for higher quality chord detection") |
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.
Line Length Violation: Line should be 120 characters or less: currently 135 characters (line_length)
| case .maj9_sus4_flat13: return "^9“4b13" | ||
| case .maj9_sus4_sharp11: return "^9“4#11" | ||
| case .maj11_sus2_flat9: return "^11“2b9" | ||
| case .maj11_sus2_sharp9: return "^11“2#9" |
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.
Switch and Case Statement Alignment Violation: Case statements should vertically align with their enclosing switch statement. (switch_case_alignment)
| case .maj7_sus2_sharp11: return "^7“2#11" | ||
| case .maj9_sus4_flat13: return "^9“4b13" | ||
| case .maj9_sus4_sharp11: return "^9“4#11" | ||
| case .maj11_sus2_flat9: return "^11“2b9" |
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.
Switch and Case Statement Alignment Violation: Case statements should vertically align with their enclosing switch statement. (switch_case_alignment)
| case .dom7_sus2_sharp11: return "7“2#11" | ||
| case .dom7_sus4_flat13: return "7“4b13" | ||
| case .dom7_sus4_sharp13: return "7“4#13" | ||
| case .dom9_sus4_flat13: return "9“4b13" |
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.
Switch and Case Statement Alignment Violation: Case statements should vertically align with their enclosing switch statement. (switch_case_alignment)
| case .dom11_sus2: return "11“2" | ||
| case .maj13_sus2: return "^13“2" | ||
| case .maj13_sus4: return "^13“4" | ||
| case .dom13_sus2: return "13“2" |
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.
Switch and Case Statement Alignment Violation: Case statements should vertically align with their enclosing switch statement. (switch_case_alignment)
| case .maj9_sus4: return "^9“4" | ||
| case .dom11_sus2: return "11“2" | ||
| case .maj13_sus2: return "^13“2" | ||
| case .maj13_sus4: return "^13“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.
Switch and Case Statement Alignment Violation: Case statements should vertically align with their enclosing switch statement. (switch_case_alignment)
| case .maj7_sus4: return "^7“4" | ||
| case .maj9_sus4: return "^9“4" | ||
| case .dom11_sus2: return "11“2" | ||
| case .maj13_sus2: return "^13“2" |
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.
Switch and Case Statement Alignment Violation: Case statements should vertically align with their enclosing switch statement. (switch_case_alignment)
| case .maj7_sus2: return "^7“2" | ||
| case .maj7_sus4: return "^7“4" | ||
| case .maj9_sus4: return "^9“4" | ||
| case .dom11_sus2: return "11“2" |
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.
Switch and Case Statement Alignment Violation: Case statements should vertically align with their enclosing switch statement. (switch_case_alignment)
| case .sus4_addSharp13: return "“4@#13" | ||
| case .maj7_sus2: return "^7“2" | ||
| case .maj7_sus4: return "^7“4" | ||
| case .maj9_sus4: return "^9“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.
Switch and Case Statement Alignment Violation: Case statements should vertically align with their enclosing switch statement. (switch_case_alignment)
Expand
ChordTypewith Comprehensive Chord Extensions and Shorthand SyntaxOverview
This PR significantly expands the
ChordTypeenum by adding nearly all available and theoretical chord extensions. While there may be some esoteric chords yet to include, this enhancement greatly increases Tonic's ability to create and identify chords.Please note: These changes are breaking. Be sure to update your code accordingly when upgrading your Tonic package.
Changes
Extensive Chord Extensions Added
ChordType.Adoption of Shorthand Syntax
Due to the unwieldy length of some chord names in the previous long-form naming convention (e.g.,
.dominantThirteenthFlatNineSharp11FlatFive), we've adopted a shorthand syntax familiar to musicians and readers of chord charts and lead sheets.Naming Conventions
Major➔majMinor➔minDominant➔domDiminished➔dimExample
.dominantThirteenthFlatNineSharp11FlatFive.dom13_flat9_sharp11_flat5Breaking Changes
ChordTypewill need to be updated to match the new naming convention.Testing
Given the substantial increase in available chords, we recognize the need for more extensive and deliberate testing.
Acknowledgments
Huge thanks to @aure for his continuous support, brilliance, and enthusiasm.