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

Add ability score increases for 5e feats #641

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

xhudaman
Copy link

@xhudaman xhudaman commented Feb 9, 2025

Adds the ability score increase to 5e feats.

Issue: #290

Copy link
Owner

@ebullient ebullient left a comment

Choose a reason for hiding this comment

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

LGTM. ;)

Tests will catch if the parsing fails entirely, but it won't catch everything.

Is there anything specific we should test for?


static AbilityScoreIncreaseFields abilityScoreIncreaseFieldFromString(String name) {
for (AbilityScoreIncreaseFields field : AbilityScoreIncreaseFields.values()) {
if (field.name().equals(name))
Copy link
Owner

Choose a reason for hiding this comment

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

equalsIgnoreCase ?

Copy link
Author

@xhudaman xhudaman Feb 13, 2025

Choose a reason for hiding this comment

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

It was to allow me to handle int with a special case since in 5eTools the string is "int" but that's reserved so I can't make that the key name in the enum. I can probably do it another way as well. This was just how I thought it up at the time.

@xhudaman
Copy link
Author

Not too sure about the testing to be honest. I could use some guidance on that.

Off the top of my head just testing different variants of the data shape and that an increase for int gets parsed and mapped to intel correctly. Beyond that, I think I would need to rubber duck it a bit before I could give a good answer.

LGTM. ;)

Tests will catch if the parsing fails entirely, but it won't catch everything.

Is there anything specific we should test for?

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