Dec 16 05:37:20 <Funkeh`> TheDanW: C_EncounterJournal.GetSectionInfo is not directly comparable to EJ_GetSectionInfo yet there is no compat added in the deprecated file?

Dec 16 05:40:30 <Funkeh`> [url]https://pastebin.com/q9T4VmaH[/url] the change to a table seems pretty irrelevant also

Dec 16 06:22:48 <Torhal> I think the table return is cleaner, personally. Don't have to use so many placeholder variables when you only care about 2 items.

Dec 16 06:29:23 <VIad> I think so too. I hope it's still performance friendly though.

Dec 16 06:36:08 <Funkeh`> Torhal: using that logic might aswell make the entire API tables

Dec 16 06:36:56 <Funkeh`> also it is not clear on some specific returns, for example the first call has a return of 12 (11th arg) which doesn't exist in the second call

Dec 16 06:37:46 <Funkeh`> anyway the place where clarity belongs is in the API docs, not returning named tables

Dec 16 06:40:16 <foxlit> blizzard’s api docs contain the same amount of info as the table keys :P

Dec 16 06:40:48 <Funkeh`> le sigh

Dec 16 06:41:47 <Funkeh`> TheDanW: [url]https://github.com/BigWigsMods/BigWigs/commit/94ea333586cbe28bcfbe3e60e83ab69a5b5f85b9[/url] until you get around to adding it to deprecated.lua so we can actually see which arg is which (yes that commit has guesswork)

Dec 16 06:42:00 <foxlit> (and depcrecated wrappers usually play it fast and loose with api semantics, so for anything complicated the benefit is mostly that your code errors one line later)

Dec 16 06:46:34 <foxlit> Both those things are regrettable, though, so hats off for trying to fix either.

Dec 18 13:11:19 <TheDanW> Funkeh`: i'll get a deprecated function in - as for returning a table that is the preferred way now, it's easier to read and maintain than select(11, SomeFunc) for example

Dec 18 17:28:51 <Funkeh`> TheDanW you're going to start changing APIs to return tables?

Dec 18 17:29:25 <Funkeh`> this does not sound efficient to me :p

Dec 18 17:37:21 <Funkeh`> I'd much prefer the efficiency of defining the variables, instead of using select, or a table, but maybe that's just me. Not to mention the flexibility of naming it whatever you want, and not not doing 10 table accesses if you're using 10 vars.

Dec 18 17:38:15 <Funkeh`> I really hope prettyness is not the reason you chose to do this

Dec 18 17:39:15 <foxlit> being able to add seamlessly add additional returns is a nice benefit, though.

Dec 18 17:39:37 <Funkeh`> you can do that fine if you're not paranoid about the order

Dec 18 17:40:04 <Funkeh`> or just change the order next expansion

Dec 18 17:41:40 <foxlit> both these approaches such. You can break existing code, or you can break logical ordering, or you can do both at different times.

Dec 18 17:42:26 <Funkeh`> the question remains is that what he wants to gain by doing this, or is it because he saw a select call and had a fit

Dec 18 17:43:47 <Simca> they started this in WoD

Dec 18 17:43:56 <Simca> all new API since WoD return tables, with a few exceptions

Dec 18 17:44:06 <Funkeh`> wat

Dec 18 17:44:29 <Simca> it's especially common amongst the C_* named stuff

Dec 18 17:44:54 <Funkeh`> Maybe you've been looking at different diffs than I've been looking at

Dec 18 17:45:22 <foxlit> maybe; who knows what you've been looking at.

Dec 18 17:45:42 <Simca> diffs don't give the full picture either, as there are several new APIs they don't use

Dec 18 17:45:55 <Simca> that is one of the main benefits of this approach

Dec 18 17:46:12 <Simca> previously we did not have any way to figure out argument names if blizzard didn't reference them

Dec 18 17:46:19 <Simca> though, i suppose that is solved now by api documentation

Dec 18 17:46:27 <Funkeh`> that's the entire point of the api docs

Dec 18 17:46:57 <Simca> this trend started before the docs existed, and i was very glad for it when i was figuring out stuff in beta

Dec 18 17:47:01 <foxlit> (most of the meaty garrison stuff returns tables. Returning 15 values seems meh.)

Dec 18 17:47:41 <Funkeh`> So it's just being used for API with large amounts of returns?

Dec 18 17:48:00 <Simca> i mean i've never seen a table return with less than 4 entries

Dec 18 17:48:30 <TheDanW> return tables isn't inefficient, it's more complicated than that, but for the most part the impact is negligible, yes you'll create more garbage but the allocation and deallocation has already been optimized, you really only have to pay more for for a more complex traversal

Dec 18 17:48:40 <TheDanW> but these aren't complex enough for that to really matter

Dec 18 17:50:23 <foxlit> evil plan: kidnap all of your nicely-allocated returns and never let them go.

Dec 18 17:50:25 * VIad has quit (Read error: Connection reset by peer)

Dec 18 17:50:36 <Funkeh`> well if you're going to start doing it for API like GetSpellInfo, I'd recommend you analyze common use cases, in that situation, you should add GetSpellName

Dec 18 17:51:37 <Funkeh`> It would be great to have that for the EJ call also, since 99% of our use case is a translated name

Dec 18 17:52:16 <foxlit> How often do you need localized tables that you can't just eat the miniscule runtime penalty for the table return? :)

Dec 18 17:52:18 <Funkeh`> C_EncounterJournal.GetSectionName

Dec 18 17:52:51 <foxlit> s/localized tables/localized names/ .

Dec 18 17:53:35 <Funkeh`> the longer it takes the longer our UI will take to draw, which if you open in combat can take some time

Dec 18 17:54:02 <Funkeh`> before the fix to loading addons in combat, we couldn't, since we hit the run time limit

Dec 18 17:55:14 <Simca> i don't know if there are ever more than like 2 spell names on the UI at once though for WA

Dec 18 17:55:32 <Funkeh`> s/UI/GUI

Dec 18 17:55:36 <Simca> but it is possible there could be some cases where it mattered slightly, like if you were doing a spell browser or something...

Dec 18 17:57:41 <Funkeh`> TheDanW what do you think?

Dec 18 17:58:18 <foxlit> Dunno. I haven't hit a case where sane API usage (i.e. not "give me all the localized text in the game, please") results in a noticable penalty from the API's choices of return types.

Dec 18 17:58:35 <Funkeh`> in the case of spell ids, we already have GetSpellTexture(id) which is nice, as it shaves a few ms off when all you want is the texture

Dec 18 17:58:45 <Funkeh`> however wanting the name is far more common :p

Dec 18 18:17:15 <TheDanW> we usually try to balance it

Dec 18 18:17:23 <TheDanW> if there's a cost to look up the data

Dec 18 18:17:27 <TheDanW> then we might return more

Dec 18 18:17:38 <TheDanW> if theres very little cost to look it up, we might break it apart more

Dec 18 18:17:58 <TheDanW> some functions have more or less thought behind them and end up returning far too much

Dec 18 18:18:02 <TheDanW> or too little

Dec 18 18:18:08 <TheDanW> but that's the general guideline

Dec 18 18:18:39 <TheDanW> in most cases localized text is cheap

Dec 18 18:18:43 <TheDanW> fwiw