Add functions for managing the Villager-style trading system.

[jb-aero]

No description provided.

[PseudoKnight]

I like this a lot more.

I think merchant_trading() implies you're getting the merchant or whether they're trading. Perhaps merchant_trader() would be better.

[jb-aero]

Oh right, we had talked about that

[PseudoKnight]

I think this is good? Have you tested this iteration? If so, I think it's good to merge.

On a side note, do we prefer boolean returns or exceptions when an id already exists when we try to add it? We haven't been consistent with that.

[jb-aero]

I did test b8996e9 before pushing, and while I'm sure I tested it before committing two weeks ago (guess I forgot to push), I did just now test to make sure merchant_trader was working.

I don't have a preference of booleans vs exceptions. Might want to get some more voices on the subject.

[LadyCailin]

I prefer exceptions in cases where things didn’t happen as the code expects, because they’re impossible to ignore. The Boolean return value can easily be ignored (in fact, that’s the default) and you may run into misleading problems later in the code, if it was expecting the previous line to work, which makes it difficult to diagnose. Exceptions are a bit more overhead, so where that matters, it’s nice to also provide a function to check beforehand if the call would work, but that has to be taken on a case by case basis.

…

On 19 Oct 2018, at 15:05, jb_aero ***@***.***> wrote: I did test b8996e9 before pushing, and while I'm sure I tested it before committing two weeks ago (guess I forgot to push), I did just now test to make sure merchant_trader was working. I don't have a preference of booleans vs exceptions. Might want to get some more voices on the subject. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[PseudoKnight]

There's also a map.put() approach where it overwrites the existing one.

The last time I added something like this was for the virtual inventories, and I threw an exception on creation where the id already existed. However, since it keeps the virtual inventory across recompiles, the script won't behave the same the second time if you didn't consider this; instead it stops entirely.

Virtual Inventories: throws exception
Scoreboards: throws exception
Bossbars: throws exception
Recipes: returns boolean

So, since this was somewhat modeled after recipes, it makes sense that it returns a boolean, but we mostly throw exceptions.

[jb-aero]

I think while initially mimicking the virtual inventory system, I switched to match the handling of trade storing, since that was modelled after recipes. My thinking now is more that the relative infrequency merchants would likely be created if created programmatically would likely not make the overhead of an exception an issue, whereas accidentally reusing a name could result in losing previous trade data and inadvertently giving someone access to something you didn't intend to, which I believe is a trickier clean-up than someone not getting something when intended. This is in contrast to other recipes which are automatically available to everyone.

TL;DR I think maybe an exception would be more appropriate after all.

[jb-aero]

I was also thinking that perhaps get_virtual_merchants should list_virtual_merchants, since it only provides their names and titles it really is more of a list function than one that gets properties or contents of things.

[PseudoKnight]

list_virtual_merchants() is inconsistent with any existing functions. We've used some combination of "get" and/or "all", like all_players(), get_scoreboards() or get_all_recipes().