Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Codable Conformance #49

Open
maksutovic opened this issue Dec 18, 2024 · 5 comments
Open

Codable Conformance #49

maksutovic opened this issue Dec 18, 2024 · 5 comments
Assignees

Comments

@maksutovic
Copy link
Collaborator

          @maksutovic @aure since we're taking a look at this PR I just wanted to keep applying pressure on Codable here.

The raw value of our ChordType is a String. If you were to change the name of a ChordType enum case in the future then you would break the decoding of any encoding of that case that you have previously made.

IMO if we are set on introducing Codable conformances, we should use some stable raw value when we do it - such as explicitly assigning each case to a specific integer value.

Originally posted by @Matt54 in #47 (comment)

I completely agree that coddle conformance needs to happen, I'll work on it this weekend

@maksutovic maksutovic self-assigned this Dec 18, 2024
@SebastianBoldt
Copy link
Contributor

SebastianBoldt commented Feb 8, 2025

If I got it right from the other discussions/thread I read,
I think the idea is more about moving in a direction where the Codable Conformance is handled by the User of the Package instead of baking in directly into the Package.

I still think that a stable Codable Conformance of the Types could also come directly from the Library if it is done well in a backwards compatible manner tbh

@Matt54
Copy link
Member

Matt54 commented Feb 8, 2025

Right, I'm not against having the package declare the Codable conformance as long as it won't break in the future. IMO using an implicitly defined string value for the raw type of the enum is not ideal because then even fixing the spelling of a case will be a breaking change.

If you explicitly tie each case to something that will not change in the future (such as a specific integer value), then you don't run into this problem.

@Matt54
Copy link
Member

Matt54 commented Feb 8, 2025

For example, let's say you changed the ChordType case "minor" to "min" or changed "min5" to "minor5".

These would be breaking changes, because the underlying raw value changes for that case. This wouldn't be true if you tied each enum case to a stable value like an integer or a specific string value vs. relying on its implicit definition.

@SebastianBoldt
Copy link
Contributor

@Matt54

This approach may work well for simple enum-based types,
but when dealing with more complex types consisting of multiple properties,
users relying on the library's Codable implementation may encounter additional problems, right 🤔

  • Adding new properties shouldn't pose any issues as long as the new property is optional.
  • Changing the name of a property can be addressed using appropriate coding keys.
  • Removing a property shouldn't lead to problems, as Codable generally ignores unused values, if I'm correct.
  • Changes to a property's type will require manual migration :/

So maybe the more complex types should be handled by the User or the Libraries ensure that
Codable implementation will always be backward compatible 🤔

@SebastianBoldt
Copy link
Contributor

SebastianBoldt commented Feb 8, 2025

I think someone needs to ...

  • ... go through all the types of the Library
  • ... define a proper Codable Strategy in a backward-compatible manner for each type

Maybe we could start with defining proper raw values for all the enum types and get rid of
the Codable annotation of the more complex types like Chord, Noteclass etc.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants