-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
ChordType Overhaul #47
Conversation
…at Five in all cases @aure
Co-authored-by: Aure <aure@aure.com>
Co-authored-by: Aure <aure@aure.com>
Co-authored-by: Aure <aure@aure.com>
Co-authored-by: Aurelius Prochazka <aure@aure.com>
Co-authored-by: Aurelius Prochazka <aure@aure.com>
Co-authored-by: Aure <aure@aure.com>
Co-authored-by: Aurelius Prochazka <aure@aure.com>
Co-authored-by: Aurelius Prochazka <aure@aure.com>
Co-authored-by: Aurelius Prochazka <aure@aure.com>
Co-authored-by: Aurelius Prochazka <aure@aure.com>
Co-authored-by: name <name@example.com>
…rity Co-authored-by: Aurelius Prochazka <aure@aure.com>
Co-authored-by: Aurelius Prochazka <aure@aure.com>
Co-authored-by: Aurelius Prochazka <aure@aure.com>
|
||
/// 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)
|
||
// MARK: - Sharp Major chords | ||
|
||
/// 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)
@@ -6,362 +6,362 @@ public extension Chord { | |||
// MARK: - Natural Major chords | |||
|
|||
/// 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)
@@ -6,362 +6,362 @@ public extension Chord { | |||
// MARK: - Natural Major chords | |||
|
|||
/// 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). |
@@ -155,7 +155,7 @@ extension Chord: CustomStringConvertible { | |||
/// Useful for custom rendering of slash notation | |||
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)
@@ -155,7 +155,7 @@ extension Chord: CustomStringConvertible { | |||
/// Useful for custom rendering of slash notation | |||
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)
@@ -278,6 +275,12 @@ extension Chord { | |||
} | |||
} | |||
|
|||
// 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. |
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!
@@ -72,6 +74,7 @@ public class ChordTable { | |||
/// | |||
/// - 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)
@@ -42,6 +43,7 @@ public class ChordTable { | |||
|
|||
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
ChordType
with Comprehensive Chord Extensions and Shorthand SyntaxOverview
This PR significantly expands the
ChordType
enum 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
➔maj
Minor
➔min
Dominant
➔dom
Diminished
➔dim
Example
.dominantThirteenthFlatNineSharp11FlatFive
.dom13_flat9_sharp11_flat5
Breaking Changes
ChordType
will 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.