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

extendBuff( aura, duration ) function for cleaner code #4114

Draft
wants to merge 10 commits into
base: thewarwithin
Choose a base branch
from

Conversation

syrifgit
Copy link
Collaborator

@syrifgit syrifgit commented Oct 30, 2024

Instead of writing buff.metamorphosis.expires = buff.metamorphosis.expires + 8

You'd write extendBuff( "metamorphosis", 8 )

Works with negative numbers, a check for the buff being removed if negative extension is greater than remaining duration, breaks out on 0.

Thoughts? What else am I missing to polish this up if you're interested in having it?

I copied it from addstack, unsure if I actually need the local a = class.auras[ aura ]?

State.lua Outdated Show resolved Hide resolved
State.lua Outdated Show resolved Hide resolved
State.lua Outdated Show resolved Hide resolved
State.lua Outdated Show resolved Hide resolved
State.lua Outdated Show resolved Hide resolved
State.lua Outdated Show resolved Hide resolved
State.lua Outdated Show resolved Hide resolved
@syrifgit
Copy link
Collaborator Author

syrifgit commented Oct 30, 2024

Review notes implemented, thanks @johnnylam88

108 ish references (some might not be duration changes) where this function could be used

image

@Hekili
Copy link
Owner

Hekili commented Oct 30, 2024

I'm open to something like this, though the name might be different (i.e., not extend) if we're anticipating using it for extensions and reductions (modifyRemains ?). Not sure yet.

Additionally, I'm curious about whether this should accomplish any of the following:

  • Shift check for aura being up into this function? That way we don't need if buff.x.up then extendBuff( "x", 2 ) end.
  • Have it return true or false depending on whether the aura was successfully modified?
    • This lets us do something like if extendBuff( "shield_block", 1 ) then do something end (successful extension).
    • Or if extendBuff( "ritual_overlord", -1 ) then do something end (reduction that wipes the aura?).
  • Consider whether we want separate functions for buffs/debuffs or whether it would make more sense to do something like extendAura( buff.battle_shout, 5 ) so we just pass the aura (table) we care about?

@syrifgit
Copy link
Collaborator Author

syrifgit commented Oct 31, 2024

Additionally, I'm curious about whether this should accomplish any of the following:

  • Shift check for aura being up into this function? That way we don't need if buff.x.up then extendBuff( "x", 2 ) end.

Sounds smart to me, cleaner code in the class files. Should we do the same for stuff like removeBuff(), removeStack()? removeStack() may already have this, but we could do a sanity check on all related functions.

  • Have it return true or false depending on whether the aura was successfully modified?
    • This lets us do something like if extendBuff( "shield_block", 1 ) then do something end (successful extension).
    • Or if extendBuff( "ritual_overlord", -1 ) then do something end (reduction that wipes the aura?).

Agree, but to clarify, "asking" if extendBuff(), it will actually apply the modification every time? It's not a "is this possible", it's a "do it, and I care if it worked or not for reasons other than the duration of the aura"

  • Consider whether we want separate functions for buffs/debuffs or whether it would make more sense to do something like extendAura( buff.battle_shout, 5 ) so we just pass the aura (table) we care about?

Either way would make sense to me, I was going to ask about debuffs after completing this one.

Will do some revising today.

@syrifgit
Copy link
Collaborator Author

syrifgit commented Oct 31, 2024

Did a pass over the function @Hekili @johnnylam88.

auraDuration( unit, aura, duration )

  • how do I make sure unit is optional? Is that possible?
  • Am I missing any checks?
  • Should a "successful" execution return true or false if the aura is removed due to the execution? Or does it simply indicate success if it worked?
  • Name can be whatever

@syrifgit syrifgit marked this pull request as draft November 19, 2024 13:08
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.

3 participants