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

ChordType Overhaul #47

Merged
merged 41 commits into from
Dec 18, 2024
Merged

ChordType Overhaul #47

merged 41 commits into from
Dec 18, 2024

Conversation

maksutovic
Copy link
Collaborator

@maksutovic maksutovic commented Oct 18, 2024

Expand ChordType with Comprehensive Chord Extensions and Shorthand Syntax

Overview

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

  • New Chord Types: Almost all known and theoretical chord types have been added to ChordType.
  • Improved Functionality: This expansion allows for more accurate and diverse chord creation and identification within Tonic.

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

  • Majormaj
  • Minormin
  • Dominantdom
  • Diminisheddim
  • Digits Instead of Spelled-Out Numbers: We now use numerical digits for chord extensions to optimize readability and manage complex types.

Example

  • Previous long-form: .dominantThirteenthFlatNineSharp11FlatFive
  • New shorthand: .dom13_flat9_sharp11_flat5

Breaking Changes

  • Naming Convention Update: The shift from long-form to shorthand chord type names means existing code using ChordType will need to be updated to match the new naming convention.
  • Action Required: Review and update your codebase to replace old chord type names with the new shorthand equivalents.

Testing

Given the substantial increase in available chords, we recognize the need for more extensive and deliberate testing.

  • Contributions Welcome: We encourage contributions to expand the test suite to cover the new chord types.
  • Testing Focus: Ensuring the correctness and reliability of chord creation and identification with the new chord types.

Acknowledgments

Huge thanks to @aure for his continuous support, brilliance, and enthusiasm.

maksutovic and others added 30 commits April 11, 2024 17:02
Co-authored-by: Aurelius Prochazka <[email protected]>
Co-authored-by: Aurelius Prochazka <[email protected]>
Co-authored-by: Aurelius Prochazka <[email protected]>
@maksutovic maksutovic requested a review from aure October 18, 2024 01:51

/// B♭ Major
static var Bb = Chord(.Bb, type: .majorTriad)
static var Bb = Chord(.Bb, type: .major)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)
Copy link

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)

@Matt54
Copy link
Member

Matt54 commented Oct 25, 2024

Ayyyyyyeeee thanks for this contribution! I only had time to quickly scan this for the moment but two things I noticed:

  1. I think you got an extra file that snuck into this - Sources/Tonic/Chord.swift.orig

  2. I don't think we should declare Codable on any of our structs or enums unless we need the conformance inside of our package. The reason is that, for example, in this case where you've simply changed the name of the enum cases on ChordType you've broken any past saves which used this. The consumer of the package can't override this (I guess they would need to make a wrapper struct for it to overcome the decoding problem - Chordable is in this boat as an application that uses ChordType within saved presets).
    I'm definitely open to hear the other side of this point. Maybe this is the Swift way of doing things - I know I've been seeing new warnings in Xcode 16 about conformances like these. But the way I see it is that because we don't depend on any particular encoding/decoding within our package we shouldn't enforce a strict way of doing so.

@aure
Copy link
Member

aure commented Dec 18, 2024

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:
Copy link

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:
Copy link

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) }

Copy link

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 }

Copy link

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)

@Matt54
Copy link
Member

Matt54 commented Dec 18, 2024

@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.

Copy link
Member

@aure aure left a 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")
Copy link

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")
Copy link

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"
Copy link

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"
Copy link

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"
Copy link

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"
Copy link

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"
Copy link

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"
Copy link

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"
Copy link

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"
Copy link

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)

@aure aure merged commit 8f103c6 into AudioKit:main Dec 18, 2024
5 checks passed
@maksutovic maksutovic mentioned this pull request Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants