Skip to content

UnknownDecodable for most enums, plus a special case for color. - #39

Merged
JacobHearst merged 3 commits into
JacobHearst:mainfrom
seansfkelley:unknowndecodable-everywhere
Aug 27, 2026
Merged

UnknownDecodable for most enums, plus a special case for color.#39
JacobHearst merged 3 commits into
JacobHearst:mainfrom
seansfkelley:unknowndecodable-everywhere

Conversation

@seansfkelley

Copy link
Copy Markdown
Contributor

Stacked on top of #38 cause I'm not sure if you want this change. After the breakage fixed in that PR I thought it would be prudent to future-proof some of the more breakage-prone parts of the type definitions.

This PR extends UnknownDecodable to almost every enumeration. It's a little bit of a nuisance to use so I made a couple editorial decisions:

  • it wouldn't be applied to Color, Rarity, Legality or ImageStatus because they shouldn't change much (if ever) and/or they are used a lot and an extra case is annoying
  • producedMana would get its own enum (because of this card) to keep the core Color type from getting dumb

I'm open to any changes. In particular, ProducedColor could also be dropped in favor of just String, or I could explicitly add ProducedMana.T instead of it just being a straight clone of Color + UnknownDecodable.

@seansfkelley
seansfkelley marked this pull request as ready for review August 25, 2026 00:06
@JacobHearst

Copy link
Copy Markdown
Owner

Stacked on top of #38 cause I'm not sure if you want this change. After the breakage fixed in that PR I thought it would be prudent to future-proof some of the more breakage-prone parts of the type definitions.

I think the extra coverage is worth it. I've been wondering about adding a macro to handle this automatically but not sure how folks feel about macro adoption. It would be nice to get rid of the rather silly CaseIterable test suite though

@JacobHearst

Copy link
Copy Markdown
Owner

Unit tests are failing because of rate-limiting, looks like my quick-n-dirty rate limiter isn't working quite right but that's something I'll have to figure out later, no reason to block this PR.

@JacobHearst
JacobHearst merged commit 0862d26 into JacobHearst:main Aug 27, 2026
1 of 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