An initiative on standardizing struct initializers

[DJBen]

As @jsbean commented on issue #98 and in README, to make this project truly usable by other third party projects, we want to expose publicly available initializers, otherwise the structs are not initializable.

Currently most of the structs are public, but lack initializers. Swift auto-synthesizes internal default initializer for us.

We never run into initialization issues that often because our tests uses @testable import, which grant internal level access of the structs. However when I try to use MusicXML as a dependency from a third party project, I get all kinds of

ClassName is inaccessible due to 'internal' protection level

Thus I want to envision a standard of initializers that we all follow, which is of course subject to debate.

---

1. Each struct should provide a default public initializer that covers all the parameters in each struct.
   1.1. For optional fields, the public initializer will provide a default value of nil.
   1.2. Alternative initializers can be kept internal for now.

2. Declare all properties const (by using let instead of var). Unless there are necessary places that requires var (which we should discuss case by case), there are no need for mutability in most of the cases. We currently use var before because we don't want to provide nil to the auto-synthesized initializer. But now we define our default initializer and default optional fields to nil, there are no reason to use var anymore.

Here's an example that sums up the standard in 1 and 2.

public struct Example {
     public let requiredField: String
     public let optionalField: Int?

    public init(requiredField: String, optionalField: Int? = nil) { 
        self.requiredField = requiredField
        self.optionalField = optionalField
    }
}

// Caller
Example(requiredField: "aaa")
Example(requiredField: "aaa", optionalField: 2)

3. Create initializer tests.
   These tests should use regular import, not @testable import, aiming to verify that each public struct is initializable publicly.

Here's an example.

import XCTest
// Do not use @testable
import MusicXML

class NoteInitializerTests: XCTestCase {
    func testPitch() {
        _ = Pitch(step: .d, octave: 3)
        _ = Pitch(step: .d, alter: 1, octave: 4)
    }
}

If we all agree, we can make improvement toward this direction and enforce how future struct are initialized. I welcome your input and comments.

[jsbean]

@DJBen, thanks for this write-up.

Each struct should provide a default public initializer that covers all the parameters in each struct.
1.1. For optional fields, the public initializer will provide a default value of nil.
1.2. Alternative initializers can be kept internal for now.

Completely agree on both counts.

Declare all properties const (by using let instead of var). Unless there are necessary places that requires var (which we should discuss case by case), there are no need for mutability in most of the cases. We currently use var before because we don't want to provide nil to the auto-synthesized initializer. But now we define our default initializer and default optional fields to nil, there are no reason to use var anymore.

Agreed here, as well. This was the initial intention, but we relaxed the mutability just for the ease of testing. I'd say once we start defining these explicit public initializers, we return the properties back to lets.

Create initializer tests. These tests should use regular import, not @testable import, aiming to verify that each public struct is initializable publicly.

This is a great idea. Do you think it's worth testing every possible combination, or just select shorthand cases and a single "all-in" case?

---

I initially saw the definition of the public API as the last priority, to be done once we verified that the types were correctly defined (i.e., for version 0.5.0). That being said, it's definitely something that can be done continuously and incrementally without issue, so we might as well get started.

Do you imagine doing this by hand, or with some sort of mechanism like Sourcery?

[jsbean]

One thing to consider here is the use of attribute groups in MusicXML. MusicXML uses attribute groups that build on top of each other, predominately for visual properties (e.g., <print-style-align>).

We haven't entirely established how we want to deal with these. Currently, they are wrapped up in nested structs (e.g., PrintStyleAlign). In the xml source, these attributes are all inherently at the top level, not "nested" as they are in our Swift source. It makes me shudder to suggest this, but we could use a class hierarchy for these (which would save us some typing), or with protocols.

I do see the visual attributes to be secondary in priority, but it is something we will have to address at some point.

[DJBen]

This is a great idea. Do you think it's worth testing every possible combination, or just select shorthand cases and a single "all-in" case?

I messed around a bit in my experimental PR and found an "all-in" case achieves best effect with minimal work.

Do you imagine doing this by hand, or with some sort of mechanism like Sourcery?

I'll start by doing this by hand; I am also open to any automation tool. Currently if there's a long list of attributes I'll write Regex to help myself.

We haven't entirely established how we want to deal with these. Currently, they are wrapped up in nested structs (e.g., PrintStyleAlign). In the xml source, these attributes are all inherently at the top level, not "nested" as they are in our Swift source. It makes me shudder to suggest this, but we could use a class hierarchy for these (which would save us some typing), or with protocols.

I strongly agree that we should nest top level attributes into structs. The reason that xml has so many top level attributes is precisely due to its inability to nest these attributes.
In my PR to fix the Positions, I invoke the nested struct's init(decoder:) in the parent struct init(decoder:) to nest the attributes effortlessly. See code How do you think of this approach?

[jsbean]

I really like the direction of implementing and calling the init(decoder:) for attribute groups like Position.

Is it possible to do the same for the Font properties?

[jsbean]

(Whoops, ignore the accidental closure)

[DJBen]

Is it possible to do the same for the Font properties?

Definitely

[jsbean]

@DJBen, my intuition is that this is relatively well implemented. Are there any cases otherwise? If no, then I'd say we can close this!

[DJBen]

Let's close this +1

…

On Wed, Oct 16, 2019 at 9:21 PM James Bean ***@***.***> wrote: @DJBen <https://github.com/DJBen>, my intuition is that this is relatively well implemented. Are there any cases otherwise? If no, then I'd say we can close this! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#101?email_source=notifications&email_token=AAP3PPJ6Y7EGLGQCS4V4F33QO7R3ZA5CNFSM4I5MGDTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBOWYGQ#issuecomment-542993434>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAP3PPIU2KLT3AI52O5TJZDQO7R3ZANCNFSM4I5MGDTA> .

-- Sihao (Ben) Lu Square, Inc. Software Engineer