Bug csend with commas acts unexpectedly when sending meat

[Irrat]

csend 1,000,000 meat to buffy

You would expect this to send 1 million meat to buffy, however the commas has this act in an unexpected manner.

Instead CLI will spam a few unexpected item found errors, and send all your meat to buffy. Only a few million meat in my case, but still a nasty surprise.

Imagine you wanted to send someone 1,000 meat?

I would expect that either the command fails when a string doesn't match exactly one item, or that commas are handled correctly.

Given the usage of commas in the command however, and the fact that this is almost not a problem aside for the "meat". It could be better to instead print an error if the formatting could be feasible as wanting to send meat instead of items; "Meat must be the first argument of the command, and not include any commas in the amount"

A lot of places do support commas, and it's not unexpected behavior for someone to use commas when inputting amounts into CLI.

Checking if someone may have typed something incorrectly should be simple as detecting digits on both sides of a comma.

 

[Ryo_Sangnoir]

Defaulting to sending all your meat in an error case seems like poor behaviour to me. I assume this is another case of 0 being interpreted as "all". I think we shouldn't do that.

 

[fronobulax]

I spent a lot of time testing StringUtilities and, in general, a lot of effort is spent to understand what the user entered as a number. There is also a pretty consistent use of zero as meaning what was passed was unparsable so the caller gets a zero.

There is code that strips out commas and there is code that understands m or M after a number as a multiplier of 1000. So depending on what is being parsed and where spaces are trimmed instead of being a delimiter, I can see several opportunities for undesired behavior. What I don't know is whether a side effect of recent changes caused this, or not.

I will investigate but busy times are coming up so it might be a matter of days before I do, just in case someone else has more time available.

 

[fronobulax]

Did a quick look and I'm pretty sure my recent work didn't create this. There has been a lot of infrastructure work on testing commands and using it will be a learning experience for me so writing a test is my next step.

 

[heeheehee]

A less scary way to reproduce is:

closet put 1,000 meat

 

[Ryo_Sangnoir]

Closet seems easier to test too.

It's in

ItemFinder.getMatchingItemList("1,000,000 meat,", KoLConstants.inventory);

which gets parsed as all your meat.

Very tempted to just make only negative numbers mean "all your meat", given the possibility of error conditions here. I'm pretty sure folk have been bitten by scripts doing "use 0 X" to mean "don't use X", but it actually means "use all X". I'd rather change the idiom.

* or -1 can still mean "all".

 

[Veracity]

I thought “*” means all and -N means all except N, for items, at least.

Perhaps that means that “-0” means all except 0.

I’d be fine if no actual number means “all” and you have to use “*”.

 

[AlbinoRhino]

Revision 19881 resolved a similar issue with storage pulls by having the command only print information to the cli in those cases. Although, I guess the "-N" behavior might be more useful outside of counting pulls.

 

[VladYvhuce]

I thought “*” means all and -N means all except N, for items, at least.

Perhaps that means that “-0” means all except 0.

I’d be fine if no actual number means “all” and you have to use “*”.

From a more layman user's stance, "*" being the only thing that means "all" is a good way to "foolproof" an accidental usage of everything. It's a lot easier to remember what not to put into commands.

 

[fronobulax]

I have a quick test and some debugger time. 1,000,000 meat gets parsed to 000 meat. 1000000 meat gets parsed to 1000000 meat. So the problem is downstream where something assumes 0 meat is all of the meat. That should be tracked down and addressed. If someone beats me to it then I'm fine. A bandage would be to make it so in this context an integer with commas is an invalid input.

 

[heeheehee]

The parse of "closet put 1,000,000 meat" is as follows:

Perform the "closet put" action with the following:
- some item matching "1" (too many to uniquely identify one)
- some item matching "000" (too many to uniquely identify one)
- "000 meat" corresponding to "0 meat", which in turn translates to all meat.

 

[Veracity]

A bandage would be to make it so in this context an integer with commas is an invalid input.

You are missing the point. When we parse from left to right, a comma means “we are looking for the next item”. What is this "context" you are talking about"? There is no “parse everything and see meat so everything before is an int”.

So an integer followed by nothing followed by a comma is bogus.

See heeheehees better analysis of how it is being parsed.

I am sure I wrote tests for ItemFinder.

Write some more tests that look at decoding numbers with commas - something I never considered - and get failures for behavior that does not agree with how they “should” operate - and fix the code until they pass.

Preferably, come to agreement first how they “should” operate.

If we are trying to parse multiple comma separated things, saying “we need to interpret commas as separating groups of thousands in an integer” - something which has NEVER been supported - i.e. is a brand new feature request - is a hard sell.

So, sell it. Give us a full proposal. Then, when we agree, write tests.

Alternatively, write tests that show an integer followed by a comma - no item or “meat” - is bogus - and fix the code to make sure it fails.

And also make sure that “0” no longer means “*”.

 

[Veracity]

csend 1,000,000 meat to buffy

You would expect this to send 1 million meat to buffy, however the commas has this act in an unexpected manner.

I would “expect” that, eh?

Question: can you send more than one thing using csend? Could you, for example, send “2 seal tooth,3 scrumptious reagent”?

In other words, more than one kind of item, each with an optional quantity, separated by comma?

If so, what does “csend 1” mean?

If not, we can continue discussion.

Instead CLI will spam a few unexpected item found errors, and send all your meat to buffy. Only a few million meat in my case, but still a nasty surprise.

I can see that. The “spamming” of “unexpected item found” should probably just abort.

You agree, right?

Imagine you wanted to send someone 1,000 meat?

I would “csend 1000 meat”. Unambiguous. Not trying to send “1 nothing” and “000 meat”

I would expect that either the command fails when a string doesn't match exactly one item, or that commas are handled correctly.

“Correctly”. Uh huh.

Yes, it should abort with invalid syntax.

A lot of places do support commas, and it's not unexpected behavior for someone to use commas when inputting amounts into CLI.

Considering that the KoLmafia has NEVER supported commas while inputting “amounts”, it is very much “unexpected behavior” for them to be ignored.

Checking if someone may have typed something incorrectly should be simple as detecting digits on both sides of a comma.

I respectfully suggest that you have no idea how input is parsed in this command.

 

[MCroft]

It gets even more fun when you try to closet numbers that do reduce to a single item. If we change this parsing, it would be good to make sure we have tests for these cases and those tests reflect our common understanding.

> closet put 334,668,001 meat
Placing meat into closet...
Placing items into closet (request 1 of 2)...
Placing items into closet (request 2 of 2)...
Requests complete.

> closet list scroll
334 scroll
668 scroll

There's also (for more fun test cases) a few items with commas in the name.

> closet put Victor, the Insult Comic Hellhound Puppet
Placing items into closet...
Transfer failed for Victor, the Insult Comic Hellhound Puppet

> closet put Notes from the Elfpocalypse, Chapter VI
Placing items into closet...
Requests complete.
> closet list Victor

> closet list Notes
Notes from the Elfpocalypse, Chapter VI

As to what 0 should mean in a CLI command, that's a good (and appropriately narrow) question.

I'd like it to mean "none", so that closet put 0 dodad was a semantic null. Currently we have "*" for "all" (and if someone wanted to make a case for actually having the word "all" as a key word meaning "all", it might be a good extension for readability). If we make 0 !="all", we could break somebody's scripted logic, but it would be fixable. I have no idea if that's a common pattern.

> closet put * ankleweights
Placing items into closet...
Requests complete.

> closet take * ankleweights
Removing items from closet...
You acquire ankleweights (2)
Requests complete.

> closet put 0 ankleweights
Placing items into closet...
Requests complete.

> closet take 0 ankleweights
Removing items from closet...
You acquire ankleweights (2)
Requests complete.

 

[fronobulax]

I need to not post when I am tired. I leave things out and then get reactions that indicate I did not make myself clear.

StringUtilites will parse user input with commas and return the expected numeric value. They also tend to return zero if the input cannot be parsed into anything meaningful. Since I have just spent time there my first response to this report was a concern that my changes were somehow responsible. Perhaps I broke some edge case in comma parsing? Perhaps the code that allowed "m" and "k" as multipliers was doing the wrong thing for "meat"? Perhaps my edge case bug fix that now returned a zero instead of an overflowed int had exposed a bug elsewhere? So my initial analysis was focused on just how much personal responsibility I had, since if I broke it, I need to fix it.

StringUtilities are not really a factor here so I think I am off the hook. Indeed StringUtilities long and int parsing operate in a context where the caller already expects the string to be numeric. At the point where the commas come into play the csend parsing is not assuming it is parsing a number.

If the goal was to make it so this never happened to a user of csend and to get that fix out as quickly as possible then csend could certainly be given special cases in parsing to handle user provided commas in a quantity of meat. But it seems pretty clear that this issue is not just with csend and it is not about parsing as much as it is about overloading the meaning of zero. I used "bandage" to suggest that something was an interim, not long term. solution motivated by a perceived sense of urgency. But I was not proposing to implement it. This is not the first time I have said what I was not going to do and confused people. I need to make it clearer what a proposed solution is and consider not posting about rejected or unlikely solutions.

I very much like the visibility and collaboration provided by reviews of PRs so I fully expect any solution I implement will have some consensus around it.

I am quite willing for someone else to take this on but I will continue until someone else submits something.

I think my first step will be to write some more tests...

 

[Veracity]

ItemFinder is complicated. I wrote an extensive test suite for it a couple of months ago. Apparently I didn’t include enough error cases - like numbers by themselves that do not have an item name. I think I did test items with a comma. Basically, our solution is that if you want one of those, it has to be the only item in the parameter list.

In any case, if we want to remove “0” as meaning the same as “*”, augmenting the tests is the place to start…

Item finder fixes by Veracity0 · Pull Request #582 · kolmafia/kolmafia

I added an extensive test suite for ItemFinder I may add to it, once coverage runs, and I can see where it falls short. Fixed ItemFinder issues: N meat -- "meat" is no longer case sensitive Fix NO...

github.com

 

[Ryo_Sangnoir]

Filed https://github.com/kolmafia/kolmafia/pull/696

Not sure whether "0 seal tooth" should return an item with a count of 0 or null. Probably should test with some actual commands. I would hope it would work either way.

 

[Veracity]

Filed https://github.com/kolmafia/kolmafia/pull/696

Not sure whether "0 seal tooth" should return an item with a count of 0 or null. Probably should test with some actual commands. I would hope it would work either way.

It should return an item with a count of 0, since it succeeded in matching the input to an item.

The question is, what do the callers do with items with a count of 0?
Will they submit a StorageRequest or ClosetRequest or whatever with that AdventureResult?
What will the requests do? Will they filter out such AdventureResults, or will they submit a request to KoL with a count of 0?

 

[fronobulax]

Filed https://github.com/kolmafia/kolmafia/pull/696

Not sure whether "0 seal tooth" should return an item with a count of 0 or null. Probably should test with some actual commands. I would hope it would work either way.

Thanks. Ball is in your court and I will get to the PR eventually.

Off the top of my head and without looking at the code if I typed "seal tooth" or "0 seal tooth" or "doesnt_parse_to_a_number seal tooth" I would expect the same response. Context matters which suggests an item with a count of zero for all three cases. We then need to see what the caller does since zero should mean "none" and not "all".

IMO and I reserve the right to be wrong after viewing the PR.

 

[Veracity]

Off the top of my head and without looking at the code if I typed "seal tooth" or "0 seal tooth" or "doesnt_parse_to_a_number seal tooth" I would expect the same response.

Default quantity is 1. I guarantee that there will be bug reports if "acquire seal tooth" stops acquiring a seal tooth because you didn't say "acquire 1 seal tooth".

I will be interested to see what "acquire 0 seal tooth" does. Hopefully immediately succeed.

I hope that "acquire not-a-number seal tooth" will say that it doesn't know what a "not-a-number seal tooth" is.

Context matters which suggests an item with a count of zero for all three cases. We then need to see what the caller does since zero should mean "none" and not "all".

Those three things have very different semantics.