Feature Support commas in items in buy/buy from mall

[philmasterplus]

@MCroft If we're after a general solution, I'm still in favor of surrounding/escape characters. It's a bigger effort than changing just one command. But it benefits every command that accepts multiple items, effects, etc.

Any clever parsing trick has pitfalls. What if there are three different items whose names are:

• Red
• Rusty Sword
• Red, Rusty Sword

How should <command> red, rusty sword be parsed? Given the nature of KoL, we can't guarantee that such ridiculous item names would never pop up in the future. A surrounding (``) or escape character (\) would provide a more robust solution.

Your concern for "pain for scripters" may be valid but overblown. If we implement surrounding/escape chars, what was broken (item names with commas passed as gCLI tokens) will be fixed. What was working (item names w/o commas passed as gCLI tokens) will still work. If users cannot be bothered to escape item names with commas themselves, we can provide an ASH function like string cli_token_escape(string name), either as a built-in or a userland function (i.e. ZLib).

 

[MCroft]

@MCroft If we're after a general solution, I'm still in favor of separators/escape characters. It's a bigger effort than changing just one command. But it benefits every command that accepts multiple items, effects, etc.

I don't get the concern for "pain for scripters". If we implement separators/escape chars, what was broken (item names with commas passed as gCLI tokens) will be fixed. What was working (item names w/o commas passed as gCLI tokens) will still work. We can even provide an ASH function like cli_token_escape() for authors, either as a built-in or a userland function (i.e. ZLib).

Any clever parsing trick has pitfalls. What if there are three different items whose names are:

• Red
• Rusty Sword
• Red, Rusty Sword

How should <command> red, rusty sword be parsed? Given the nature of KoL, we can't guarantee that such ridiculous item names would never pop up in the future.

What I mean by pain for scripters is I don't want to make them responsible for remembering to wrap a variable in quotes e.g. buy "Red, Rusty Sword", "Rusty Sword", "Red", "{AnotherItemInAVarible}". I feel like it's worth avoiding things that require any script that uses this feature to need an update to take advantage of it.

I feel like there's a can of worms built in to quotes, even though there are lots of first class languages that use them for this purpose. Does it work like bash, where we allow variable expansion if it's a double-quote, but not if it's a single quote? What if they introduce an item with curly braces and variables are broken? What's the proper behavior if the parameters are wrong? There's lots to handling that well. That doesn't mean it's not worth doing, just that it's not a 10 line fix.

Even if we (or you, if you feel that strongly) are in favor of using quoted strings, I think the verified_split() method is the way to tackle this, because that gives us a way to build it with whatever method and apply it to any list-based command.

 

[philmasterplus]

When I suggested backticks, I wasn't even considering variable substitution (i.e. Python's f-strings or JavaScript's template literals). I simply chose them because single- and double-quotes are already used in several item names. Variable substitution in gCLI is an unrelated topic. No need to go down the Bash rabbit hole.

This patch feels like a band-aid fix that adds maintenance burden down the road. I understand that scripting support for the Commerce Ghost is strongly and urgently desired, but please leave some wriggle room for the future even if you don't wish to fill them later. If nothing else, I suggest adding some comments explaining your rationale, expected inputs for the happy path, and perhaps a // TODO.

P.S. What is "verified_split()"? I searched KoLmafia's codebase, the wiki, this forum, and the ASS discord but couldn't find any mention of it. My apologies. I didn't read the last part of your post.

 

[MCroft]

Ok, so here's my intent. I'm going to take a crack at a utility function to create an array of parameters, to be used where we split parameter strings now. It should be easy to do both a backtick system and a string concat system as a fallback.

It should support a quote character pulled from KoLConstants, initially set to backtick. If there are no backticks, it will try to split on ",", and it will use concatenation if it can't find the unticked items.

It won't handle the "red, rusty sword" hypothetical situation without backticks, but it doesn't break the existing items that don't have that problem and it has a way to use other behavior if it turns out it should. Also should be very easy to extend. And I'll try to document it strongly.

If someone else wants to write ash mallbuy(), that would make sense for the ghost. I don't think there's any attachment to the CLI for the ghost, but there's not a way to force mall buying when the NPC is cheaper in ASH.

 

[fronobulax]

Sometimes a working system with a documented bug is better/more reliable/safer than the results of fixing the bug. There, I'm done with the philosophy argument.

I think a user should not have to know that they need to do one thing if an item name has a comma and something else if it doesn't. So I'd favor a solution that allows/requires KoLmafia to figure it out.

If I am typing in a cli and get an error I might add an escape character and resubmit but if I am scripting then I am going to just always add the escape because that is easier than figuring out whether I need to add the escape or not.

Problem only occurs when there is user generated input that is presented as a comma delimited list of strings that must be parsed to generate a list/array of the things that are represented by the strings.

The most common things will be Items, so a splitter that returned a list of items seems like a big first step. I would expect it to split on commas, and check each field. If a two adjacent fields don't make an item then I would concatenate them and see if the result made an item. This concatenation check needs to be iterative or recursive in case an item has multiple commas in the name. If a field is not an item or part of a concatenation that makes an item then there is an error. If because of fuzzy matching, the field does not generate a unique item then I would also generate an error, I think it is safer to abort the entire command if there is a parameter error then try and execute with only the valid parameters.

So I think I am lining up in a corner with a variation of verified_split as a KoLmafia internal which is called every place user input is a list of comma delimited strings.

 

[Malibu Stacey]

As suggested in the ASS Discord by @Rinn and @MCroft I can workaround the issue that caused this thread by doing "buy from mall [itemid]" (the square brackets are very much required there) which removes any ambiguity. I will fully admit I had no idea this functionality existed until now and am very pleased to have learned something.
As the thing I'm trying to automate buying comes from a property set by KoLMafia it's simple enough to call .to_item().to_int() on the string returned by get_property() to get the item id required.

Thanks all.

 

[philmasterplus]

As suggested in the ASS Discord by @Rinn and @MCroft I can workaround the issue that caused this thread by doing "buy from mall [itemid]" (the square brackets are very much required there) which removes any ambiguity. I will fully admit I had no idea this functionality existed until now and am very pleased to have learned something.
As the thing I'm trying to automate buying comes from a property set by KoLMafia it's simple enough to call .to_item().to_int() on the string returned by get_property() to get the item id required.

Thanks all.

I'm probably beating a dead horse, but I discovered that pilcrows (¶) were used in the olden days to feed raw item IDs to gCLI commands. Have we reinvented the wheel?

(I discovered this while examining OCD Inventory Control, btw.)

 

[MCroft]

I've felt much less urgency since we found that "if you have a single item, you should convert it to the ID and put it in brackets" workaround.

But I haven't given up on it.

 

[fronobulax]

Given that archeologists (thanks) have rediscovered the existence of a pilcrow as a cli delimiter is there anything to do? It seems to me that the solution is to tell scripters how to pass lists of items with commas rather than add code to deal with commas in item names. If there is a case where the pilcrow doesn't work as expected that would then be a Bug or Feature Request to deal with that specific command.

 

[dark_adonis]

Given that archeologists (thanks) have rediscovered the existence of a pilcrow as a cli delimiter is there anything to do? It seems to me that the solution is to tell scripters how to pass lists of items with commas rather than add code to deal with commas in item names. If there is a case where the pilcrow doesn't work as expected that would then be a Bug or Feature Request to deal with that specific command.

It doesn't seem like the Pilcrow is ultimately changing things since you are still providing the list of items by id rather than name. If the pilcrow method doesn't work we still have the workaround that Rinn/MCroft established. I guess the stance could be that we have a way of mall buying so should we add additional burden to be able to handle commas/ambiguities?

 

[MCroft]

It doesn't seem like the Pilcrow is ultimately changing things since you are still providing the list of items by id rather than name. If the pilcrow method doesn't work we still have the workaround that Rinn/MCroft established. I guess the stance could be that we have a way of mall buying so should we add additional burden to be able to handle commas/ambiguities?

From the wiki link

send 1 ¶4358 to Bale|Thanks for being awesome
is a way to send A Crimbo Carol, Ch. 5 to Bale despite the comma in the item's name

It's the same as buy from mall [4358], but it's not a delimiter based solution, just a way to indicate that something is an ID. My guess is that these were different generations of solutions for the same problem. Who knows? The next person who finds this may try guessing that if the string is not found and it can be converted to a number, it must be an ID, and then we'd have 3 solutions...

> buy from mall [1]

Searching for "seal-clubbing club"...
Search complete.
Purchasing seal-clubbing club (1 @ 100)...
Purchases complete.

> buy from mall ¶1

Searching for "seal-clubbing club"...
Search complete.
Purchasing seal-clubbing club (1 @ 100)...
Purchases complete.

So, it could still benefit from being more robust in terms of string handling on item-not-found, and there's still a use case for mall_buy(qty, item, price). Not sure when I'll get to it, but I think you all helped me find a good approach.