There are some vendor items in the game that are listed by Blizz as "Miscellaneous: Companion Pets", but they aren't. (Most of them are toys.)

C_PetJournal.GetNumCollectedInfo (line 228 in DataFunctions.lua) is not happy with that at all, since the speciesID is nil, and throws the appropriate errors, for example:

2x MerchantPlus/DataFunctions.lua:228: Usage: GetNumCollectedInfo(speciesID)
[string "=[C]"]: in function `GetNumCollectedInfo'
[string "@MerchantPlus/DataFunctions.lua"]:228: in function `func'
[string "@MerchantPlus/DataFunctions.lua"]:46: in function `GetData'
[string "@MerchantPlus/MerchantPlusItemList.lua"]:110: in function `RefreshScrollFrame'
[string "@MerchantPlus/MerchantPlus.lua"]:401: in function <MerchantPlus/MerchantPlus.lua:374>

Locals:
(*temporary) = nil

As a result we get things like this:

Screenshot

I have created a pull request for that, und you find the details there.

– Tom

Did you find any cases where the false companion pet was not a toy but was something else that could be collected?

Yes, as mentioned in the detailed text there is one: the consumable "poor man's" version of the Void Tendril Pet Leash toy. It's noted as a "one-time use item" in the list (ID 174925). I haven't found anything else of that sort, but admittedly I wasn't really searching for more ;)

I don't know if 174925 is collectible, but I tend to think that not. (I've read your comment in the file: "This is a consumable! Sometimes these can be collected".)

I could just check if the pet species is nil

Yes, probably more elegant and future-proof against more false pets. (Unless it is a consumable and collectible that should not just fall through to the end of the branch.)

Does that seem like a reasonable approach?

Hey, it's your addon :)

Over most of the time I used the addon with just one false-pet item excluded (no list, just testing for the ID in the condition). Only after I stumbled upon a second broken "pet" I searched more systematically and added the found ones as a little list instead. Also, I found it preferable not to call GetPetInfoByItemID twice (or for all Miscellaneous items) but that probably doesn't matter too much…

Did you find any cases where the false companion pet was not a toy but was something else that could be collected? I'm thinking that rather than keeping a static list (something I've been attempting to avoid) I could just check if the pet species is nil. If so, I'd attempt to see if it's a toy, then if it's neither, assume it can't be collected. Does that seem like a reasonable approach?

I don't know if 174925 is collectible, but I tend to think that not. (I've read your comment in the file: "This is a consumable! Sometimes these can be collected".)

Consumables that can be collected are usually one-shots like Ensembles and Illusions though it's tricky to test for them, so the only logic I apply to that is whether it's an item you can preview in the dressup window or if it's flagged as known. Off the top of my head I can't remember any "persistent" objects that have a learned component, but that seems like it would be contradictory. Generally if you can add an item to your collections it serves no purpose to continue to live in the merchant's inventory or your own and will vanish or be marked as "Already Known" and no longer be able to be used.

Hey, it's your addon :)

Yep, but since you took the time to work up and test a PR I was interested in anything you might have discovered that made that approach unsound! :)

Over most of the time I used the addon with just one false-pet item excluded (no list, just testing for the ID in the condition). Only after I stumbled upon a second broken "pet" I searched more systematically and added the found ones as a little list instead. Also, I found it preferable not to call GetPetInfoByItemID twice (or for all Miscellaneous items) but that probably doesn't matter too much…

I don't think GetPetInfoByItemID on non-pets is too expensive, but it's true that the more functions I call on each item in the merchant, the slower it will be to load some of the larger inventories (renown quartermasters), especially the first time after login.

Consumables that can be collected are usually one-shots like Ensembles and Illusions

Ah ok.

Yep, but since you took the time to work up and test a PR I was interested in anything you might have discovered that made that approach unsound! :)

Originally, I posted the PR like this because I am lazy: I just took the modification 1:1 as I was using it in my personal working copy of the addon. (When I modify someone else's addon for personal use, I try to keep it minimally invasive, just to avoid conflicts when merging future author updates into it.)

However, the second thought was indeed that the table is the more economic way, and frankly I would leave it with the hardcoded list — unless we or other people find huge amounts of similar items (or Blizz maliciously keeps adding more ;)

Not sure if it is related at all to this function, but recently I noticed that there was a noticeable delay(and sort of flickering?) in rebuilding the merchant listing when buying an unlearned BoP transmog (i.e. when the appearance is learned while the merchant frame is open, and hence the "Collectable" column gets updated).

Originally, I posted the PR like this because I am lazy: I just took the modification 1:1 as I was using it in my personal working copy of the addon. (When I modify someone else's addon for personal use, I try to keep it minimally invasive, just to avoid conflicts when merging future author updates into it.)

A good practice, for sure!

However, the second thought was indeed that the table is the more economic way, and frankly I would leave it with the hardcoded list — unless we or other people find huge amounts of similar items (or Blizz maliciously keeps adding more ;)

I'm inclined to agree with this stance. I think the likely story is that Blizzard originally intended some of these items as pets when they were added to the database and later added functionality that made them more appropriate as toys but just didn't think to update the categories. For the pet leashes, it might be that they were "related" so the person who input them figured that was the appropriate category. This probably won't happen often.

In any case, I think I'll merge the PR, but also add a check because I don't want to throw a LUA error anyway. If the check fails, I'll print a message to the console asking the user to consider reporting the fake pet so I can add the exception.

Not sure if it is related at all to this function, but recently I noticed that there was a noticeable delay(and sort of flickering?) in rebuilding the merchant listing when buying an unlearned BoP transmog (i.e. when the appearance is learned while the merchant frame is open, and hence the "Collectable" column gets updated).

When you learn an item like that it fires an event, probably UNIT_INVENTORY_CHANGED or MERCHANT_UPDATE and I can't really tell what changed, so I have to assume that I need to refresh the whole merchant list. It might flicker if the game throws one or both of these events several times in short succession, which I've seen happen. If this is a frustrating problem (might be on larger inventory merchants), the solution is probably to queue the refresh with a short timer instead of executing it immediately to allow all the events to fire ahead of it.

but just didn't think to update the categories.

Yes, and then the eternal Blizz tragedy: nothing ever gets fixed unless it causes 100 forum posts a day for 7 consecutive days :(

In any case, I think I'll merge the PR, but also add a check because I don't want to throw a LUA error anyway. If the check fails, I'll print a message to the console asking the user to consider reporting the fake pet so I can add the exception.

Oh, this sounds like a very good idea. If you do the nil check with petinfo[13] after line 238, it's free :)

Since DMF is online, I verified the unverified Flimsy Yellow Balloon (75042), and as expected, it causes the issue if I comment it in the list:

 
At the occasion, I found 2 more false pets at this merchant: Flimsy Darkmoon Balloon and Flimsy Green Balloon that would also cause the issue if not excluded:
 

 
(BTW, the other items in the screenshot – the ones without "Flimsy" in the name – are real pets.)

Unlike the Flimsy Yellow Balloon, the 2 new false pets are not Toys, but one of those annoying toy-like items that take up bag space:

 
What a mess :)

Here the updated list:

local falsePets = {
	[37460] = true, -- Rope Pet Leash (verified): Toy
	[44820] = true, -- Red Ribbon Pet Leash (verified): Toy
	[174995] = true, -- Void Tendril Pet Leash (verified): Toy
	[174925] = true, -- Void Tendril Pet Leash (verified): One-time use item
	[71137] = true, -- Brewfest Keg Pony (wowhead); Brewfest event only: Toy
	[75040] = true, -- Flimsy Darkmoon Balloon (verified); DMF event only: Toy-like bag item
	[75041] = true, -- Flimsy Green Balloon (verified); DMF event only: Toy-like bag item
	[75042] = true, -- Flimsy Yellow Balloon (verified); DMF event only: Toy
}

I tested and verified my nil check code with those additional items and have also added them to the list. The current code in git includes those changes and another fix for transmog armor that I noticed a while back. That release will go out with the patch on Tuesday.

Great. Thank you!

And Thank You for this good addon!

– Tom

PS: I just went to the main pet vendor at the DMF (Lahra), and there aren't any issues, only real pets and mounts :)

Thanks again for reporting the issue and the PR. I'm always happy to see other people enjoying the addon!

Thanks for reporting this and for the PR. I'll try to look it over and merge it by this weekend!