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

Foundry 12 / DnD5e expects the term to have operator or number set #1144

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

AlanWaiss
Copy link

In troubleshooting Beyond20 with Foundry v12, I found that the terms expect an "operator" for "OperatorTerm" terms or "number" for "NumericTerm" terms.

I'm also only adding the + OperatorTerm if the previous term isn't already an OperatorTerm and using foundry.utils.isNewerVersion when available since the global isNewerVersion is deprecated.

Issue 1127

@SethStanowski
Copy link

Just tested this on my Foundry VTT server works great, needed this fix thank you

FVTT Version: 12
Build: 328
Game System: dnd5e v 3.3.0

tested on chrome version 126.0.6478.182 (Official Build) (64-bit)

Copy link
Owner

@kakaroto kakaroto left a comment

Choose a reason for hiding this comment

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

Finally got the time/wits to review this. My main worry is if it causes issues on 0.8.x to v11, so I'll want to test it out first before merging, but code wise, it looks good and makes sense to me.
It looks like the change in v12 is that they stopped using expression and now use operator or number fields instead, for some reason. Wondering if it could have been fixed just by adding those two fields. (i believe multiple + operators in succession isn't an issue, but i like your solution to it)
The only possible issue is that i don't remember what those roll parts might contain and if we could have two formula parts with no operators in between since you remove the autoadd of operator, and only apply it during adding a numeric value.
Will test and if nothing breaks on older versions, I'll merge.
Thanks again and apologies for the delay in getting it looked at!

@kakaroto kakaroto merged commit e66419a into kakaroto:master Jul 26, 2024
@kakaroto
Copy link
Owner

Thanks, it's merged! I tested on v9 and it turns out even in v9, it was using operator and number 🤦 I've slightly cleaned it up and merged!

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.

None yet

4 participants