#1290 Consolidate getplayerbuff into an api method

[dsidirop]

https://github.com/shagu/pfUI/issues/1290

[shagu]

I always tried to move as much of vanilla-vs-tbc checks out of the code and use the compat ones instead. But this is straight the other way around, moving even more vanilla-vs-tbc checks into the API code.

Also your IDE changed the formatting from 2 spaces to 4 spaces in the whole api.lua and also reformatted code blocks that have nothing to do with the PR itself.

I haven't checked the details yet, but if TBC already has a GetPlayerBuff() that we can use without modifications, it would be better to implement a version in compat/vanilla.lua that works the same. (Like done with hooksecurefunc and such: https://github.com/shagu/pfUI/blob/master/compat/vanilla.lua#L36-L80)

[dsidirop]

But this is straight the other way around, moving even more vanilla-vs-tbc checks into the API code. I haven't checked the details yet, but if TBC already has a GetPlayerBuff() that we can use without modifications, it would be better to implement a version in compat/vanilla.lua that works the same.

Good points. I thought of doing that but bare in mind that there is a huge caveat that I already pointed out:

If I do override 'GetPlayerBuff' itself inside compat/vanilla.lua to make it behave like the tbc one then pfUI will work but any third party pfui-extension out there that uses 'GetPlayerBuff()' expecting it to work as in vanilla will immediately break upon updating pfui to the latest version because they expect 0-based indeces but with this patch we'll be going behind their backs enforcing 1-based indeces on 'GetPlayerBuff()'.

Here's what I mean in code:

setfenv(1, pfUI:GetEnvironment())  -- 3rd party addons import the pfUI environment

-- they naively use 0->31 because they aim only for vanilla stuff
-- this works for them but it will break once I patch GetPlayerBuff()!
-- it's very hard for them to figure out what broke and to change it to    for i=1,32,1   which is the tbc way of doing things!
for i=0,31,1 do 
       local b = GetPlayerBuff( i, "HELPFUL" )
       [...]
end

We can avoid all of this if I create under compat/vanilla.lua and compat/tbc.lua a side-flavour called 'GetPlayerBuffX()'. In TBC it will be set simply to 'GetPlayerBuff' but in vanilla it will be polyfilled in the same fashion as shown in this patch.

This way we won't cause any problems to the existing ecosystem that imports 'setfenv(1, pfUI:GetEnvironment())'.

Let me know your thoughts on this tradeoff.

[shagu]

Well, that makes sense.. If I'd had to pick one, the 2nd approach seems to be the best solution, where we have PlayerBuffX in both compat/vanilla and compat/tbc.

I just wonder right now if it's even worth doing it: After all, it will be more lines of code, but with the same functionality as we already have. It just saves the user (mod developer) from placing a static variable into it, right? A mod developer must still know the client difference otherwise he wouldn't even think of using GetPlayerBuffX instead of GetPlayerBuff. And at that point, he could also place the static variable in.

The next question we should ask ourselves is where do we stop? Are we going to build wrapper functions for every single constant variable in the compat module? To be honest, I have the fear to get lost in wrapper functions and the bloat around it.

What are your thoughts about that?

[dsidirop]

To be honest, I have the fear to get lost in wrapper functions and the bloat around it.

I'm afraid of the same thing. This is why I sided with frugality and only introduced the bare minimum of changes which insofar is just the first argument of GetPlayerBuffX() while keeping its return values the same as before. Seems to be the only thing we're gonna be needing for many years to come.

Regarding the naming is 'GetPlayerBuffX()' a good naming convention? Or is something like (just thinking out loud) 'PFGetPlayerBuff()' better? (implying that if any future compat-utils will also be named as PF[something] for the same reasons we've uncovered during this analysis)

Spend some time thinking about what naming convention rings true to you and let me know what you've decided.

[shagu]

Hey, I have decided to not merge this in. Thanks for your efforts tho. But I do prefer the solution with static variables over the suggested one with wrapper functions.