Feature Support commas in items in buy/buy from mall

MCroft

MCroft

Developer
Staff member
So, items with commas in the description are a pain when the user sends a string of comma separated parameters to the function: eg:

buy from mall Notes from the Elfpocalypse, Chapter VI, herbs

Are we buying 3 items or two? It's hard for scripters, because they're not able to drop the errant comma and let fuzzy matching rule the day.

What I've got in this patch
1: notices when there's no match
2: adds the current itemName to the next itemName in the next round of the for loop
"Notes from the Elfpocalypse" + ", " + Chapter VI"
3: If that's null, try falling back to just "Chapter VI", because maybe the miss was a real miss.

But the results are sorta chatty, and might look like there was an error.

I'm open to suggestions for ways to handle it better.

I would also like to understand a bit more of the difference between "mallbuy" and "buy from mall", because it's not really clear to me what's happening with that. It may be acting strange because I'm in ronin...

Rich (BB code): 
> buy from mall Notes from the Elfpocalypse, Chapter VI

notes from the elfpocalypse, chapter i
notes from the elfpocalypse, chapter v
notes from the elfpocalypse, chapter vi

[Notes from the Elfpocalypse] has too many matches.
Searching for "Notes from the Elfpocalypse, Chapter VI"...
Search complete.
Purchasing Notes from the Elfpocalypse, Chapter VI (1 @ 200)...
Purchases complete.


> buy from mall Notes from the Elfpocalypse, Chapter VI, herbs

notes from the elfpocalypse, chapter i
notes from the elfpocalypse, chapter v
notes from the elfpocalypse, chapter vi

[Notes from the Elfpocalypse] has too many matches.
Using cached search results for Notes from the Elfpocalypse, Chapter VI...
Purchasing Notes from the Elfpocalypse, Chapter VI (1 @ 200)...
Purchases complete.
Searching for "herbs"...
Search complete.
Purchasing herbs (1 @ 100)...
Purchases complete.

> buy from mall squidbait, herbs

[squidbait] has no matches.
[squidbait, herbs] has no matches.
Searching for "herbs"...
Search complete.
Purchasing herbs (1 @ 100)...
Purchases complete.

> buy from mall Notes from the Elfpocalypse, Chapter VI, squidbreath, herbs

notes from the elfpocalypse, chapter i
notes from the elfpocalypse, chapter v
notes from the elfpocalypse, chapter vi

[Notes from the Elfpocalypse] has too many matches.
Searching for "Notes from the Elfpocalypse, Chapter VI"...
Search complete.
Purchasing Notes from the Elfpocalypse, Chapter VI (1 @ 200)...
Purchases complete.
[squidbreath] has no matches.
[squidbreath, herbs] has no matches.
Searching for "herbs"...
Search complete.
Purchasing herbs (1 @ 100)...
Purchases complete.

> mallbuy Notes from the Elfpocalypse, Chapter VI, squidbreath, herbs

notes from the elfpocalypse, chapter i
notes from the elfpocalypse, chapter v
notes from the elfpocalypse, chapter vi

[Notes from the Elfpocalypse] has too many matches.
[squidbreath] has no matches.
[squidbreath, herbs] has no matches.
Desired purchase quantity not reached (wanted 1, got 0)
 

Attachments

If things that were supposed to be items were coerced to item numbers or the canonical(?) form [1234]I just made this up, I think would that be another path?
 
fronobulax said:
If things that were supposed to be items were coerced to item numbers or the canonical(?) form [1234]I just made this up, I think would that be another path?
Click to expand...
I thought about that. Maybe we need a second form of "buy" which takes a number instead of a string. They're still running in to "pull the name of the item from the ghost, and buy from mall $item[ Notes from the Elfpocalype, Chaper VI ]”, but if they were to look up the item and then use buy from mall $ItemNum, it would be non-ambiguous.

I also thought about escaping the commas or enclosing the params in quotes, but I was pretty sure I'd break something that lots of things depend on.

I'm guessing that when "buy" was introduced, there weren't items with commas in the name.
 
Perhaps allow backticks as wrapping characters? They are never used in item names (unless they release more stuff like the Baby Bugged Bugbear famequips).

Code: 
> buy from mall `Notes from the Elfpocalypse, Chapter VI`

Ofc this is coming from someone who writes JavaScript every day and sprinkles backticks everywhere. Also keep in mind that Python devs did reject the backtick due to accessibility reasons among other things.
 
philmasterplus said:
Perhaps allow backticks as wrapping characters? They are never used in item names (unless they release more stuff like the Baby Bugged Bugbear famequips).

Code: 
> buy from mall `Notes from the Elfpocalypse, Chapter VI`

Ofc this is coming from someone who writes JavaScript every day and sprinkles backticks everywhere. Also keep in mind that Python devs did reject the backtick due to accessibility reasons among other things.
Click to expand...
What I was initially trying to support was Autoscend's use of buy from mall for the commerce ghost

Code: 
    string ghostItemString = get_property("commerceGhostItem");
    string output = cli_execute_output(buy from mall {ghostItemString});
    if (!output.contains_text("Purchases complete."))
    {
        abort(Something went wrong buying {ghostItemString} from the mall.);
    }

@Malibu Stacey said "I'd rather have an ash function I can pass a $item to and not care about any of this", where "this" was "wrapping items in quotes, escaping things, etc."

Right now comma is the special character. I'd worry about introducing a new special character (e.g. backtick or backslash or quotes), when I can handle it with just some retries around the comma. And it's a lookup against an in-memory data structure, so it's not appreciably slower to do it the way I'm mentioning.

Adding any kind of balanced delimiters would need error checking, would require changes to existing scripts to support the Notes from the Elfpocalypse, and wouldn't be future-resilient (none of it is future-proof...).

All of which could be done, of course. And we could do both things.

Beyond the problem I'm solving, what advantages do you see to doing it with balanced backticks?
 
Oh. I thought this was a general gCLI usability issue. So the actual problem is the lack of a scriptable "buy from mall, bypassing the vendor check" feature?

In that case, I agree that a new function (or an overloaded form of buy()) would be better.
 
philmasterplus said:
Oh. I thought this was a general gCLI usability issue. So the actual problem is the lack of a scriptable "buy from mall, bypassing the vendor check" feature?

In that case, I agree that a new function (or an overloaded form of buy()) would be better.
Click to expand...
Sort of. It's not about "bypass the vendor check", that's already there with the magic phrase "buy from mall" as the command and first two parameters. It's about the rest of the predicate.

buy works in 99.99% of cases to buy things like herbs, spices, bottle of gin, whatever. But it fails when the item actually has a comma in the name. It can't tell the difference between item 1, item 2, with a comma, item 3. Is that 4 items or 3?

The patch says "if you fail on item 2, on the next pass use item 2+", " + with a comma.

So, it's probably a CLI usability issue as well as an issue for scripters with that item, but in the gCLI you can deal with the error by modifying your request or using the store gui, so it's not a major gCLI usability issue that came up very much.

The patch is to the regular CLI "buy" command. I can't test very fully (in hardcore for a couple more days) and I don't love the output, so I wanted to solicit opinions.
 
Maybe we revert the addition of "from mall" to the buy cli command and consider an ash function instead?

From reading https://wiki.kolmafia.us/index.php/Buy I would check to see what happens if the existing buy command is used with the optional price and the price is higher than then NPC price. The documentation is ambiguous but it would be worth confirming the capability isn't already there.

If not I would add a a version of buy that let the caller force the purchase to be from the mall, or fail.

I feel like we are making this harder than it needs to be although I am prepared to be wrong about that feeling.
 
I feel like we’re conflating two issues.
1: how to force mall buying for a ghost that must buy a thing from the mall. @gausie did that and it works.
2; how any buy request tells the difference between a comma separated list item when there’s a comma in the name.

I don’t want to solve #1, it’s done and works. I want to solve #2 without breaking gausie’s enhancement fir #1
 
Why is a solution better than telling users who want to buy multiple items to make a list in ash and pass them to buy one by one? I don't see any new functionality being added and sometimes documenting a known problem is better than fixing it. I focused on mall buy because that is the only case I could think of where - don't do that, use ash - would not apply.

If you are going to do it anyway you might consider splitting on commas because reconstructing an item name by concatenating successive fields from a matcher might lose white space.
 
Let me try to clarify the actual failure that's happening because we're overloading "," as both a character in an item name and a list separator.

they're reading an item in a script and then sending $item[thing with, comma]

buy assumes that predicates are a comma separate list of items to buy one at a time.

It's treating thing with, comma as buy thing with then buy comma.

Here's the script I'm trying to make work regardless of the comma:
Code: 
string ghostItemString = get_property("commerceGhostItem");
    string output = cli_execute_output(buy from mall {ghostItemString});
    if (!output.contains_text("Purchases complete."))
    {
        abort(Something went wrong buying {ghostItemString} from the mall.);
    }

If property commerceGhostItem is "spices", it works.
If propertycommerceGhostItem is "Notes from the Elfpocalypse, Chapter VI", it fails to buy "Notes from the Elfpocalypse".

Does that make sense?

If there's a better way to use the property that's a string with a comma in it and buy the item, I'm open to the suggestion.
 
Years ago, I changed item matching internally (it was for the drink command, but I made it more general) so that if the first segment failed to match an item, the entire string was checked for a matching item. Without looking at any code, I expect the same type of fix would work well here.

Back then, the response to "what about 'item1 with a comma, item2, item3'?" was "don't do that.
 
MCroft said:
Let me try to clarify the actual failure that's happening because we're overloading "," as both a character in an item name and a list separator.

they're reading an item in a script and then sending $item[thing with, comma]

buy assumes that predicates are a comma separate list of items to buy one at a time.

It's treating thing with, comma as buy thing with then buy comma.

Here's the script I'm trying to make work regardless of the comma:
Code: 
string ghostItemString = get_property("commerceGhostItem");
    string output = cli_execute_output(buy from mall {ghostItemString});
    if (!output.contains_text("Purchases complete."))
    {
        abort(Something went wrong buying {ghostItemString} from the mall.);
    }

If property commerceGhostItem is "spices", it works.
If propertycommerceGhostItem is "Notes from the Elfpocalypse, Chapter VI", it fails to buy "Notes from the Elfpocalypse".

Does that make sense?

If there's a better way to use the property that's a string with a comma in it and buy the item, I'm open to the suggestion.
Click to expand...

I understand the problem. There is a reason a tab is a delimiter in most KoLmfia data files.
 
So, given that buy is probably well-established as item1, item2, item3, ... I think we have a small number of approaches

  1. Do nothing. It is not possible to buy "Notes from the Elfpocalypse, Chapter VI" in a script, but you can remove "," from your script and it'll probably work
  2. Create a new command (e.g. "Buyone"). Refactor "buy" so it sends each item to "buyone". If you only one want one, use buyone.
  3. Create a new command, buy int, int, int. It is the responsibility of the scripter to convert the item to an int before calling buy. Will fail if they ever create an item with all digits as the name.
  4. Add string handling markers or escape characters (e.g. "\\," or "Notes from the Elfpocalypse, Chapter VI". Some complexity ensues, including pain for scripters.
  5. make an ash function that takes Items or item arrays and forces mall buying and returns the quantity bought.
  6. @lostcalpolydude 's approach: if item 1 is a miss, try the string-as-a-whole-unit
  7. my approach: if any item is a miss, try adding it back to the next item specifically to find string-with-comma items. The patch in this thread does that in 10 lines of code. It might fail on "Notes from the Elfpocalypse,[two spaces]Chapter VI"
@fronobulax, I think you're advocating for #5, but I wasn't really sure.
 
1 then 5.

Focus the support on ash not the cli. Perhaps an ash command would be more useful to the JS scripters than a cli command?
 
fronobulax said:
1 then 5.

Focus the support on ash not the cli. Perhaps an ash command would be more useful to the JS scripters than a cli command?
Click to expand...
I know that's a lot of the devs' focus, but it isn't always my focus. I don't write scripts or generally use them, and I like making KoLmafia better for users like me, too.

It turns out that I think the use case in question would be better served by an ash command that allowed the scripter to buy only from the mall, even if the NPC price was lower. Someone might well refactor buy(item) to buy(Item item, Bool fromMall, Bool withStorage, Bool fromCoinmaster) and re-write the existing functions as convenience functions. Or something like that.

Even if we did that, I still don't want to leave buy from mall item1, item2, with comma, item3 as a broken function. Even if you don't use it, I'd like it to work.
 
MCroft said:
Even if we did that, I still don't want to leave buy from mall item1, item2, with comma, item3 as a broken function. Even if you don't use it, I'd like it to work.
Click to expand...

It's a philosophy discussion at this point. It has been broken for years and what broke it was the KoL bug that introduced a common delimiter, comma, into an item name.
 
fronobulax said:
It's a philosophy discussion at this point. It has been broken for years and what broke it was the KoL bug that introduced a common delimiter, comma, into an item name.
Click to expand...
Ok. I accept your two points and still want to change it.

I am in favor of fixing things that are broken, even if they've been broken for years. KoL is an evolving game and this evolved from "there are no items with commas" to "there are a few items with commas, but not important ones" to "there's a thing that needs to buy an item with a comma from a particular source". The next iteration might make it required to buy one, so having it broken means one more thing that could go bad. The cost of leaving this defect in has risen. Maybe not to the level that you'd chose to work on it (maybe not even to the level where you want to talk about it, tbh :)), but it's one more thing that's slightly more robust if we fix it.

I'm really never posting on these forums to get discouragement from making changes that aren't dangerous.
 
Last edited:
MCroft said:
Even if we did that, I still don't want to leave buy from mall item1, item2, with comma, item3 as a broken function. Even if you don't use it, I'd like it to work.
Click to expand...
Is that important to you just for this function, or would you want to expand it to other comma-separated CLI commands?
 
lostcalpolydude said:
Is that important to you just for this function, or would you want to expand it to other comma-separated CLI commands?
Click to expand...
I think it would be better to solve it generally, which leads to a different kind of function.

Hmm. some sort of utility function to take a string of parameters and a type and return an array of validated types. Then we add it as we get to it to buy, chew, drink, eat, autosell, up, etc. The validator wouldn't produce gCLI output, but the user can tell how many items they got back by the array size.

It would replace String[] itemNames = parameters.split( "\\s*,\\s*" );
with something like String[] itemNames = UtilityFunctionClass.verifiedSplit( parameters, ",", KoLConstants.ITEM_TYPE );
 
Last edited:
You must log in or register to reply here.
Back
Top