dn-m / MusicXML

Implementation of the musicXML specification in Swift
MIT License
70 stars 19 forks source link

[WIP / Experimental / Do Not Merge] Use @dynamicMemberLookup #173

Closed jsbean closed 4 years ago

jsbean commented 4 years ago

This PR tries out the usage of KeyPath-based @dynamicMemberLookup for percolating the attributes of certain attribute groups (PrintStyle, PrintStyleAlign, Position, etc.) up to the top-level of a structure which holds onto such an attribute group.

In the first applicable commit (268557a), we can use it like this:

let sharp = Accidental.sharp
let defaultX = sharp.defaultX

This reaches down two levels: Accidental -> PrintStyle -> Position. I chose not to expose the properties of Font because the font. prefix contextualizes the members' names (.style, etc.).

@DJBen, @bwetherfield, let me know if this seems dangerous or superfluous to you.

(I will get rid of the commit trash shortly.)

jsbean commented 4 years ago

If it suits your fancy, perhaps one of you Sourcerers could cook up a .stencil to blast the code base ⚗️.

bwetherfield commented 4 years ago

I'm curious if this could also be achieved through computed variables + sourcery in a more type-safe manner? Do you require setter functionality?

codecov-io commented 4 years ago

Codecov Report

Merging #173 into latest will increase coverage by 0.8%. The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff            @@
##           latest     #173     +/-   ##
=========================================
+ Coverage    67.9%   68.71%   +0.8%     
=========================================
  Files         221      221             
  Lines        3546     3609     +63     
=========================================
+ Hits         2408     2480     +72     
+ Misses       1138     1129      -9
Impacted Files Coverage Δ
Sources/MusicXML/Simple Types/Tenths.swift 70% <ø> (ø) :arrow_up:
Sources/MusicXML/Complex Types/PrintStyle.swift 81.81% <0%> (-18.19%) :arrow_down:
Sources/MusicXML/Complex Types/Accidental.swift 93.54% <0%> (-6.46%) :arrow_down:
...cXML/Complex Types/Partwise/Partwise.Measure.swift 100% <100%> (ø) :arrow_up:
...ces/MusicXML/Complex Types/Partwise/Partwise.swift 100% <100%> (ø) :arrow_up:
...ces/MusicXML/Complex Types/Timewise/Timewise.swift 78.12% <100%> (+36.94%) :arrow_up:
...cXML/Complex Types/Timewise/Timewise.Measure.swift 88.88% <87.5%> (+88.88%) :arrow_up:
Sources/MusicXML/Complex Types/StyleText.swift 100% <0%> (ø) :arrow_up:
...urces/MusicXML/Complex Types/MusicXML.String.swift 100% <0%> (ø) :arrow_up:
Sources/MusicXML/Complex Types/Key.swift 71.91% <0%> (+0.39%) :arrow_up:
... and 3 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 2933e55...268557a. Read the comment docs.

bwetherfield commented 4 years ago

I'm also down to have a protocol for this type of sub-struct (PrintStyle, Position etc.), because they also all require direct decoding / encoding (PrintStyle(from: decoder) and printStyle.encode(to: encoder) syntax, or using singleValueContainers)

jsbean commented 4 years ago

AFAIU, the KeyPath variant of @dynamicMemberLookup is type safe (the original String variant of @dynamicMemberLookup not being so). The let-ness of the properties cuts off the setter functionality.

I'd argue it's pretty equivalent to a codegen solution, but with less potential for getting out-of-sync in case anything changes.

jsbean commented 4 years ago

I'm also down to have a protocol for this type of sub-struct (PrintStyle, Position etc.), because they also all require direct decoding / encoding (PrintStyle(from: decoder) and printStyle.encode(to: encoder) syntax, or using singleValueContainers)

This is interesting. How would we hook into such a solution so that we can optimally cash in 💸 ?

bwetherfield commented 4 years ago

Seems probably fine then. What's the catch?

jsbean commented 4 years ago

I don't think there is one … 👻 ?

bwetherfield commented 4 years ago

kewl!

bwetherfield commented 4 years ago

Would this be a 0.4.0 change as opposed to 0.3.0?

jsbean commented 4 years ago

Definitely a 0.4.0 change. I have to say I got excited and distracted by shiny things 😬 .

jsbean commented 4 years ago

Let's revisit this another time.