Feature - Implemented More friendly warnings, this time for duplicate names

zarqon

Well-known member
Presently, $item[Staff of Ed] and to_item("Staff of Ed") returns the second of the two Staves of Ed without mentioning that it could have just as well returned the first staff. And similarly for the two ancient amulets.

I would like to request that mafia display a "friendly warning" in cases where the supplied constant name has multiple matches. For example, a script that contains $item[ancient amulet] should print something like

Multiple matches for "ancient amulet" (somescript.ash, line 123). Clarify by using one of:
$item[2180]
$item[7963]

And a script that uses to_item("Staff of Ed") should print something like:

Multiple matches for "Staff of Ed" (somescript.ash, line 123). Clarify by using one of:
"[2325]Staff of Ed"
"[7961]Staff of Ed"

Ideally, these warnings would also fire for loading data files, since that's another case where text is matched up to mafia's constants.
 

Darzil

Developer
I think my search is broken. Can only find the word "clarify" three times in all the Mafia build files, and none in that context.
 

heeheehee

Developer
Staff member
Amusing note when digging a bit into this: to_item("[1]hamsters from outer space") will resolve to $item[seal-clubbing club], as the tag after the brackets is entirely ignored.

There seem to be a few issues at play here:

* to_item invokes (at some point) ItemDatabase.getCanonicalName, which in turn looks if the item is present in ItemDatabase.itemIdByName.
* When populating that map, we happily clobber any existing values without checking for collisions. That's probably why we had so many issues with $item[rock] and $item[2108] (and associated items).

I imagine we could populate a "name collision" map as well (mapping String itemname -> int[] ids), and simultaneously set the itemIdByName entry to some invalid item ID. Haven't really put too much thought into exactly how that'd work, just yet.
 

Veracity

Developer
Staff member
Presently, $item[Staff of Ed] and to_item("Staff of Ed") returns the second of the two Staves of Ed without mentioning that it could have just as well returned the first staff. And similarly for the two ancient amulets.
$item[] is a compile-time lookup. to_item() is a run-time lookup - although in the case of a constant string argument, we could optimize it into the compile-time constant.

Code:
[color=green]> ash to_item( "[2325]Staff of Ed" ).to_int()[/color]

Returned: 2325

[color=green]> ash to_item( 2325 ).to_int()[/color]

Returned: 2325
Seems to me that if you know the item id, you could just do to_item( 2325).

When loading files, presumably we are converting strings into "item" objects.
And when writing files, we'd need to know that Staff of Ed and ancient amulet are both ambiguous and include the item id.

This is all connected with DataTypes.parseItemValue() which has the following code:

Code:
		// Otherwise, let ItemDatabase parse the name using fuzzy matching.
		int itemId = ItemDatabase.getItemId( name );
ItemDatabase.getItemId resolves this via:

Code:
ItemDatabase.itemIdByName.get( name )
Which is to say, a single map lookup. There is no knowledge of which items have ambiguous names. Although, I imagine we could determine that when loading the data file.
 

Veracity

Developer
Staff member
I imagine we could populate a "name collision" map as well (mapping String itemname -> int[] ids), and simultaneously set the itemIdByName entry to some invalid item ID. Haven't really put too much thought into exactly how that'd work, just yet.
For no obvious reason - given current code - itemIdByName is defined to be a Map<String, Object>, even though we currently only ever map to Integers - and we assume that what is there is an Integer.

Seems like we could make it a Map<String,int[]>. In the vast majority of cases, the array would be size 1, but in the handful of places where there is a name collision, it would have more than 1.

ItemDatabase.getItemId() would return the last element in the array (for compatibility).
ItemDatabase.getItemIds() would return the array. That is the method we'd need to do the kind of stauff zarqon wants.
 

zarqon

Well-known member
Appreciate all the swift responses. Not sure if it's helpful but I'd like to suggest that it may be useful to do this in a way that could apply universally to any of KoLmafia's typed constants, not just items. Though to my knowledge, these two items are presently the only duplicates.

Feature was requested because I have a script that ran into some difficult-to-debug errors a while back due to KoLmafia essentially changing items on me without letting me know. And from several different sources -- one from a data file (where the item was a field in a map of records), and one hardcoded in a script (using to_item(string)).
 

Veracity

Developer
Staff member
I'd like to suggest that it may be useful to do this in a way that could apply universally to any of KoLmafia's typed constants, not just items. Though to my knowledge, these two items are presently the only duplicates.
There are multiple effects with the same name, too. A Little Bit Evil, for example. We'd need to be able to, similary, translate an effect name into a set of effect ids.
 

Veracity

Developer
Staff member
I coded up a change to ItemDatabase to let it deal with items that have multiple item ids:

Code:
[color=green]> test itemids seal tooth[/color]

seal tooth has 1 itemid: 2

[color=green]> test itemids Staff of Ed[/color]

Staff of Ed has 2 itemids: 2325, 7961

[color=green]> test itemids ancient amulet[/color]

ancient amulet has 2 itemids: 2180, 7963
For now, I left it such that when KoLmafia (anywhere) asks for the item ID of an ambiguously named item, it gets the last one entered:

Code:
[color=green]> ash to_item( "Staff of Ed" ).to_int()[/color]

Returned: 7961
I think I'll submit this to let any issues with it shake out before deciding where/how to use it.

ItemDatabase int getItemId( String name, int count, boolean substringMatch )
(and the overloaded variants) should operate as before.

ItemDatabase int[] getItemIds( String name, int count, boolean substringMatch )
is the new method that returns all IDs. getItemId now uses this to do all the heavy lifting.
 

Veracity

Developer
Staff member
Revision 17520 does the same for effects:

Code:
[color=green]> test effectids Got Milk[/color]

Got Milk has 1 effectid: 211

[color=green]> test effectids A Little Bit Evil[/color]

A Little Bit Evil has 6 effectids: 597, 598, 599, 600, 601, 602
We now have the infrastructure to do the kind of thing this Feature needs.
 

Veracity

Developer
Staff member
So, I have a little experimental change. I made the "name" of any item or effect that has an ambiguous name be the [XXX]NAME form, where XXX is the id. That means that such objects will print in that form. You can also use to_string() on that name and it will work.

Code:
[color=green]> ash $items[ seal tooth, 2325, [7961\]Staff of Ed, ancient amulet ][/color]

Changing "ancient amulet" to "[7963]ancient amulet" would get rid of this message ()
Returned: aggregate boolean [item]
seal tooth => true
[2325]Staff of Ed => true
[7961]Staff of Ed => true
[7963]ancient amulet => true

[color=green]> ash $item[ staff of ed ].to_string()[/color]

Changing "staff of ed" to "[7961]Staff of Ed" would get rid of this message ()
Returned: [7961]Staff of Ed

[color=green]> > ash $item[ [7961]staff of ed ].to_string()

Bad item value: "[7961" ()[/color]
Returned: void

[color=green]> ash to_item( "[7961]Staff of Ed" )[/color]

Returned: [7961]Staff of Ed
plural => Staff of Eds
...
1) Since to_string() of such an object gives you the [XXX] form, and to_item() of a string with [XXX] gives you the correct object, this automatically does the right thing for map_to_file and file_to map. At least, new ones; an existing file with an ambiguous item or effect will still give the item or effect with highest item number.
2) In $item[], if you want to use [XXX]NAME, the "]" closes the constant. You can escape it with a \, but that's sort of inconvenient. We should either say:

"Use [XXX\]NAME to make this message go away", or fix $item[] parsing to allow the nested [XXX] without an escape. I think I like the latter, but have not implemented. Comments are welcome.

3) It would obviously be easy to give an alternate message when you have multiple matching items or effects, as zarqon suggested.
 

zarqon

Well-known member
Thanks to this change, I was made aware that there are two Spookyraven library keys. :)

The current message could be slightly misleading if the cause is a use of $item[], since using $item[[id]name] fails with an error.
 

Veracity

Developer
Staff member
Revision 17526 should allow you to use [XXX]YYY in a $item[] or $items[] (or $effect, etc.) without error. Ergo, you don't need to use a "" to quote the "[".
 

Veracity

Developer
Staff member
Code:
[color=green]> ash $item[ ancient amulet ].to_string()[/color]

Multiple matches for "ancient amulet"; using "[7963]ancient amulet". () Clarify by using one of:
$item[[2180]ancient amulet]
$item[[7963]ancient amulet]
Returned: [7963]ancient amulet

[color=green]> ash to_item( "ancient amulet" ).to_string()[/color]

Multiple matches for "ancient amulet"; using "[7963]ancient amulet". () Clarify by using one of:
"[2180]ancient amulet"
"[7963]ancient amulet"
Returned: [7963]ancient amulet

[color=green]> ash $effect[ A Little Bit Evil ].to_string()[/color]

Multiple matches for "A Little Bit Evil"; using "[602]A Little Bit Evil". () Clarify by using one of:
$effect[[597]A Little Bit Evil]
$effect[[598]A Little Bit Evil]
$effect[[599]A Little Bit Evil]
$effect[[600]A Little Bit Evil]
$effect[[601]A Little Bit Evil]
$effect[[602]A Little Bit Evil]
Returned: [602]A Little Bit Evil

[color=green]> ash to_effect( "A Little Bit Evil" ).to_string()[/color]

Multiple matches for "A Little Bit Evil"; using "[602]A Little Bit Evil". () Clarify by using one of:
"[597]A Little Bit Evil"
"[598]A Little Bit Evil"
"[599]A Little Bit Evil"
"[600]A Little Bit Evil"
"[601]A Little Bit Evil"
"[602]A Little Bit Evil"
Returned: [602]A Little Bit Evil
That's in revision 17531.

The $item[] and $effect[] warnings are issued at script parsing time. the to_item() and to_effect() warnings are issued at script execution time.

I did not validate item & effect values read from files since when we write those objects to a file using map_to_file(), we always include the id in square brackets first - just in case KoL later introduces a duplicate name.
 

zarqon

Well-known member
Beautiful! Thank you.

I did not validate item & effect values read from files since when we write those objects to a file using map_to_file(), we always include the id in square brackets first - just in case KoL later introduces a duplicate name.

Most of the data files I use are made by hand as a way to supply data to the script, not written by mafia as script output. They do not have the brackets.

I suppose I can fix that by loading and resaving all my maps.
 

Veracity

Developer
Staff member
I suppose I could validate the data when loading. I did have an implementation of that, but when I looked at the generated data file, I saw that every item or effect had brackets, and decided, "why bother?" But, yes, I can see why a hand-generated data file would not have that.

Let me consider that again.
 

Veracity

Developer
Staff member
OK. Given this test script:

Code:
boolean[item] imap2;

file_to_map( "imap2.txt", imap2 );

print( "" );
print( "read item map" );
foreach it in imap2 {
    print (it );
}
and this imap2.txt

Code:
seal tooth	true
Staff of Ed	true
2325	true
I now get:

Code:
[color=green]> test-constants3.ash[/color]

Multiple matches for Staff of Ed; using [7961]Staff of Ed in (imap2.txt, line 2). Clarify by using one of:
[2325]Staff of Ed
[7961]Staff of Ed

read item map
seal tooth
[2325]Staff of Ed
[7961]Staff of Ed
Note that the map is sorted by the "key" - the item, which is to say, by itemId - rather than being in the order of lines in the file.
Note also that the file & line number in the warning are for the data file, not the script.

Revision 17543
 
Top