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 chance to DisplayText also #621

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Conversation

Rubio9
Copy link

@Rubio9 Rubio9 commented Aug 21, 2023

[Contributing a patch I've kept locally for a couple years]

Add "chance" to DisplayText, so the same calculated chance (displayed as "Likelihood" in the GUI list) will also display in chat log (or wherever else this is sent).

@rdw-software
Copy link
Member

Heyo, thanks for the contribution! I love this idea, but there's a few things we should consider. I'll add review comments for them.

Copy link
Member

@rdw-software rdw-software left a comment

Choose a reason for hiding this comment

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

The localization needs to be fixed, and ideally the chance computation extracted (for testability). Otherwise, looks good to me :)

Edit: The formatting is also off, but I'll fix that if you don't want to bother with it.

Core/Announcements.lua Outdated Show resolved Hide resolved
Core/Announcements.lua Outdated Show resolved Hide resolved
Core/Announcements.lua Outdated Show resolved Hide resolved
@rdw-software rdw-software merged commit 81d0861 into WowRarity:master Aug 31, 2023
5 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