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

V2 expose enums #425

Merged
merged 36 commits into from
Mar 20, 2024
Merged

V2 expose enums #425

merged 36 commits into from
Mar 20, 2024

Conversation

augustjohnson
Copy link
Collaborator

Exposing a few utility classes, and the enums.

@calumbell
Copy link
Contributor

Hi, thanks again for your work!

This is a pretty substantial PR which touches on several existing issues, but it doesn't include much context on what has been changed. Best I can tell, this looks like a refactor of several data models to redefine certains fields as enums. Is that the basic shape of it?

Also, it looks like something has happened to the spell schools in the v2 API data. They all read as being from the school of Evocation?

@augustjohnson
Copy link
Collaborator Author

Eh sorry about that, that wasn't even up to my (admittedly low) standards of PR documentation.

The goal of this PR was overall, to expose a set of values for well-known and re-used fields. I also wanted to consolidate hardcoded values to a single place. Some of these remain hardcoded (but now centralized), and some I have written into custom models.

Size is the best example of a custom model. Size was being stored as an integer before, but within the scope of this PR was to make a clear "size" that was shared as a field across various models (object, creature). In that particular case, my solution was to create (and expose) a full object and add some fields. Similarly, I adjusted Spell Schools, and Damage Types and Item Rarities. As a note, there is not a Rank field on both Item Rarities and Size that can be used to sort.

But a few other items I just centralized into a hardcoded values section (called enums). Now there's also a /enums endpoint, which exposes some of the "hardcoded" values used, like Create Attack Types (Weapon, or Spell), and Spell Target Type Choices, (creature, object, point, area).

Functionally, this has very little changes on the endpoints/models that are existing in v2, even though those files were edited.

Regarding all spells being evocation, yes, that's a bug of some sort. I'll fix it.

@augustjohnson
Copy link
Collaborator Author

Fixed. Did you know that a5e has a distinct spell school called "Transformation"? I didn't! But luckily the app was forgiving enough to clearly show me the problem, and to allow me a simple solution.

Copy link
Contributor

@calumbell calumbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thanks for taking the time to explain what is going on in this PR, it was very helpful! Refactoring these hard-coded/enums values so that there are all in one spot seems like a good design choice to me.

I have had a poke around the source code and nothing strange jumps out at me. Tested the PR out on local and everything appears to run as expected. Good work!

@augustjohnson augustjohnson merged commit e3575d8 into staging Mar 20, 2024
3 checks passed
@augustjohnson augustjohnson deleted the v2_expose_enums branch June 1, 2024 17:48
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