wardz / DRList-1.0

[WoW] Library for providing player diminishing returns categorization.
16 stars 10 forks source link

[Feature] Cata shared dr-categories (deep freeze + ring of frost) #19

Closed XiconQoo closed 5 months ago

XiconQoo commented 6 months ago

As you mentioned in https://github.com/wardz/DRList-1.0/blob/master/DRList-1.0/Spells.lua#L828 already, a spell having two categories is something DRList cannot handle right now.

My suggesetion is sth like this:

function Lib:GetCategoryBySpellID(spellID)
    if Lib.gameExpansion == "classic" then
        -- OBSOLETE: second return value is no longer needed after Classic Era patch 1.15.0.
        local category = Lib.spellList[spellID]
        if not category then return end
        return category, spellID
    end

    local category = Lib.spellList[spellID]
    if type(category) == "table" then
        return unpack(category)
    else
        return category
    end
end

This would initially not break addons using DRList, considering they don't use DRList.spellList.

wardz commented 6 months ago

Yeah, was thinking about doing something like this as a quick fix, but kinda annoying that addons would have to check for the extra return values manually. I'm taking it most addon authors dont read lib changelogs. Also if there's ever more than 2 categories returned they'd have to use something like select("#", category) to get the total count which hurts performance. (Unless the second return value is always an array/table only containing its shared DR categories, then you could just check if the array exist and use the # operator directly. First return value would still work as normal for GetCategoryBySpellID as we'd just return category[1] value)

Guess there's not much else we can do since we cant break DRData or older DRList compability either. I'll take a further look into it this weekend when i get the time.

wardz commented 5 months ago

Mostly completed now in v1.5.0.