-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fix Key.NonTraditional model and decoding #120
Conversation
Codecov Report
@@ 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.
|
@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? |
See my latest PR #124, I encountered the same issue with 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 |
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?)
} |
@bwetherfield yeah that would work. In practice I recommend better error handling logic than
|
@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 |
Resolves #119.