Fix Key.NonTraditional model and decoding

[jsbean]

Resolves #119.

[codecov-io]

Codecov Report

Merging #120 into latest will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           latest     #120   +/-   ##
=======================================
  Coverage   45.16%   45.16%           
=======================================
  Files         218      218           
  Lines        2555     2555           
=======================================
  Hits         1154     1154           
  Misses       1401     1401

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 017d3ba...1e3e837. Read the comment docs.

[jsbean]

@DJBen & @bwetherfield, this is a bit of a tenuous fix. As per the spec, it would be possible to see something like this:

<key>
    <key-step>B</key-step>
    <key-alter>-1</key-alter>

    <key-step>E</key-step>
    <key-alter>-2</key-alter>
    <key-accidental>slash-flat</key-accidental>

    <key-step>A</key-step>
    <key-alter>-2</key-alter>
    <key-accidental>slash-flat</key-accidental>

    <key-step>F</key-step>
    <key-alter>2</key-alter>
</key>

With this fix, this example would not parse correctly because of this maneuver.

Any thoughts on how to handle this better?

[DJBen]

See my latest PR #124, I encountered the same issue with harmony having imploded group that is a list: harmony-chord.

I've tried many decoder combinations but none avail. My final solution is to parse them into a list of enums like below

enum KeyComponent {
  case keyStep(KeyStep)
  case keyAlter(KeyAlter)
  case keyAccidental(KeyAccidental)
}

and reassemble it into multiple Keys. (see my implementation)

[bwetherfield]

That looks good! Would something like this be possible to enforce order?

while !valuesContainer.isAtEnd {
    do {
        keyComponents.append(.keyStep(try valuesContainer.decode(KeyStep.self)))
        keyComponents.append(.keyAlter(try valuesContainer.decode(KeyAlter.self)))
        keyComponents.append(.keyAccidental(try valuesContainer.decodeIfPresent(KeyAccidental.self)))
    } catch {
        break
    }
}

This would work with

enum KeyComponent {
  case keyStep(KeyStep)
  case keyAlter(KeyAlter)
  case keyAccidental(KeyAccidental?)
}

[DJBen]

@bwetherfield yeah that would work. In practice I recommend better error handling logic than catch { break } because this catches both errors

• Wrong key: this is what we want to catch, so we can start parsing next parameters.
• Decoding failure: this is what we not want to catch.

[bwetherfield]

@DJBen - 100%. This would be a great adjustment for the other cases we have used this "start parsing next parameters" fix (I noticed this in the PR you referenced too, looks like a great practice). In this particular case, perhaps any kind of catch { break } is overkill; would probably want to just throw at the top level?