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

Fix Stabat Mater test #126

Merged
merged 7 commits into from
Oct 11, 2019
Merged

Fix Stabat Mater test #126

merged 7 commits into from
Oct 11, 2019

Conversation

jsbean
Copy link
Member

@jsbean jsbean commented Oct 9, 2019

This PR fixes the Schubert Stabat Mater LilyPond test. Down to 51 LilyPond test failures.

@codecov-io
Copy link

codecov-io commented Oct 9, 2019

Codecov Report

Merging #126 into latest will increase coverage by 2.43%.
The diff coverage is 96.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           latest     #126      +/-   ##
==========================================
+ Coverage   57.67%   60.11%   +2.43%     
==========================================
  Files         218      219       +1     
  Lines        2800     2911     +111     
==========================================
+ Hits         1615     1750     +135     
+ Misses       1185     1161      -24
Impacted Files Coverage Δ
Sources/MusicXML/Complex Types/Direction.swift 100% <100%> (ø) ⬆️
Sources/MusicXML/Complex Types/FormattedText.swift 100% <100%> (ø) ⬆️
Sources/MusicXML/Complex Types/Offset.swift 100% <100%> (+100%) ⬆️
...ources/MusicXML/Complex Types/TextDecoration.swift 100% <100%> (+100%) ⬆️
Sources/MusicXML/Complex Types/Dynamic.swift 100% <100%> (ø)
Sources/MusicXML/Complex Types/Dynamics.swift 100% <100%> (+100%) ⬆️
Sources/MusicXML/Complex Types/DirectionType.swift 42.15% <60%> (+0.32%) ⬆️
Sources/MusicXML/Complex Types/Percussion.swift 0% <0%> (ø) ⬆️
Sources/MusicXML/Complex Types/Technique.swift 50.42% <0%> (+0.84%) ⬆️
... and 6 more

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 9ec7650...ab047d9. Read the comment docs.

@jsbean
Copy link
Member Author

jsbean commented Oct 9, 2019

@DJBen, @bwetherfield, I need to bail for the evening. I am also a bit stuck on this one. If either of you want to pick this up, that would be great!

This will require getting the Dynamic / Dynamics / DirectionType / Direction pipeline under control. There are some failing tests to get started.

@jsbean jsbean changed the title [WIP] Fix Stabat Mater test Fix Stabat Mater test Oct 11, 2019
@jsbean
Copy link
Member Author

jsbean commented Oct 11, 2019

@DJBen, @bwetherfield, this fix constrains things a little bit more than the spec re: Dynamics.

In the spec, a DirectionType.dynamics should theoretically be able to hold onto a sequence of Dynamics values. I wasn't able to get that to decode properly. We may need to nest this array into a custom type or do some other (de-|re-)nesting procedure in the decode process. This fix only allows you to have a single Dynamics value per DirectionType.

Several LilyPond tests are fixed by this, so I am ok with this being a stop-gap solution. There is a #warning(FIXME:) in the source, and I will create an issue to track this.

@@ -36,5 +38,44 @@ public enum Dynamic: String {
//case other(OtherDynamics)
}

extension Dynamic: XMLChoiceCodingKey { }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long live XMLChoiceCodingKey.

@jsbean jsbean merged commit 5eee362 into latest Oct 11, 2019
@jsbean jsbean deleted the fix-stabat-mater branch October 11, 2019 18:45
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.

None yet

2 participants