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

feat: added yellow border color #27

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

missingems
Copy link
Contributor

No description provided.

@missingems
Copy link
Contributor Author

missingems commented Jan 23, 2025

Circling back to this discussion: #26 (comment)
In hindsight, I agree with you, @JacobHearst. that we should keep these dynamic types as raw strings. We can still maintain the enum, but perhaps place it inside a helper (e.g., Card+Enums). This way, we can update the helper to map to the new type if needed. And, when WotC decides to release a new type, at least we won’t fail to decode it. We could add a new case .unknown(rawValue: String) for new types we haven't implemented.

I severely underestimated the creativity from WotC.

@JacobHearst
Copy link
Owner

JacobHearst commented Jan 23, 2025

I severely underestimated the creativity from WotC.

So did I when I started this library 😂 . We do currently have unknown cases (with no associated value) for Card.Layout and Card.FrameEffect, I think it would be good to add the raw value in there as you suggested. It will be a breaking change though so I'd like to go through the library and see if there's any other improvements that would break API compatibility. Then we can just group them in the same major version instead of incrementing the major version twice.

As for this PR, what card adds this yellow border color? The smoke tests didn't catch this so I'd like to figure out why and hopefully remedy that

@missingems
Copy link
Contributor Author

The new set Aetherdrift has a new yellow border type and I am getting decoding error out of it.

@JacobHearst
Copy link
Owner

Okay figured out the reason this wasn't caught. I'll get a fix onto main after I merge this PR

@JacobHearst JacobHearst merged commit 6bb42f3 into JacobHearst:main Jan 24, 2025
2 checks passed
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.

2 participants