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

Added enchantment, shatteredglass, upsidedowndfc frame effects #26

Merged

Conversation

missingems
Copy link
Contributor

Added some of the missing frame effects https://scryfall.com/docs/api/frames

@missingems
Copy link
Contributor Author

@JacobHearst

@JacobHearst JacobHearst self-assigned this Nov 20, 2024
@JacobHearst
Copy link
Owner

Thanks @missingems! I saw the alerts about the failed builds but haven't had time to actually get around to fixing them. I've been considering for a while whether these frequently updated enums even need to exist or if they should just be strings, do you have any opinion on this?

@missingems
Copy link
Contributor Author

Thanks @missingems! I saw the alerts about the failed builds but haven't had time to actually get around to fixing them. I've been considering for a while whether these frequently updated enums even need to exist or if they should just be strings, do you have any opinion on this?

I personally think we can keep the enum as it is and update it occasionally. The cycle from WotC is also fairly predictable. Personally, I rely on the different frame effects to render various types of shaders. Removing the enum would mean we’d have to switch to using strings instead, which shifts the responsibility of ensuring each frame effect value is correct onto the implementer.

I can also help contribute more often if things becoming too hectic from Wotc (hopefully not).

@JacobHearst
Copy link
Owner

Sounds good to me, I'll get this merged and released!

@JacobHearst JacobHearst merged commit 7e3ea0f into JacobHearst:main Nov 21, 2024
2 checks passed
@missingems missingems deleted the Add-additional-frame-effect branch November 22, 2024 00:09
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