Feature JSON

[fronobulax]

In Ye Olden Days KoLmafia did not really support the use of third party jar files and the solution was to copy source code. That has changed and we can drop in jar files and be done with it. I noticed we had source from json.org in our project and that there was a corresponding jar file. I propose adding a jar file (json-20210307.jar) and deleting the copied source.

I grabbed the jar from https://mvnrepository.com/artifact/org.json/json/20210307 and links there can get you to GitHub and source.

All tests pass locally with my change (which is why I added one that uses JSON) and I will run with changes for a day or two.

I think this is a good idea but I can be convinced otherwise

 

[gausie]

Sounds good to me!

 

[fronobulax]

Thanks. Momentarily on hold since I had errors running this morning. It is probably a good thing to have something more concrete, and perhaps actually useful, than coverage++ ;-)

 

[heeheehee]

It looks like JSONObject.getString() actually changed behavior at some point, such that it'll throw an exception if the field isn't a string field.

Used to be that it was just implemented as get(key).toString(), until, um, 2011.

(I tried running something comparable locally, but ran into ApiRequest parsing issues thanks to limitmode:0 )

 

[fronobulax]

limitmode was reported. I haven't dug in enough to decide whether KoLmafia's code was locally changed, or an ancient jar file would make the problem go away or whether the JSON provider was actually providing "bad" JSON and we never cared until now.

And while I wasn't paying attention, something I did made the IDE think JSONException no longer needed to be explicitly included so exception handling may have changed.

Since this fits my definition of Good Things To Do I will track it down.

 

[heeheehee]

Ancient jar would work, as would changing that one callsite in CharacterPaneRequest to use get( "limitmode" ).toString(). (This code looks largely untouched since Veracity added it in 2010, other than a move to lib/ and some override annotations by roippi in 2012)

api.php has probably always given us 0 in that field. It's JSON, not structured data with strict types for fields...

 

[heeheehee]

There's a related cleanup to embed the swingx jar. Some build.xml changes seem like they're needed, since it's used in both lib/ and src/. Probably the lib/net/java/dev/spellcast stuff should live in src, or we should consider moving away from that in favor of more modern APIs...

And, it seems like jdom / sequence in lib/jar are both unused, as well. Would be nice to not have to manually add / remove jars...

 

[fronobulax]

I will make some decision about JSON and implement it.

I will be glad to look at swingx next.

When I last opened this can of worms, I could not find a jar with spellcast. Isolating the code files in their own source tree was a flag that said "this is third party code that we are using but do not wish to change or maintain". So, it is easy to imagine a time when there is no source in lib/ because of upgrading, refactoring and finding jars.

I'll check the jdom and sequence as well. I seem to recall adding them for a reason but don't remember why.

When I was living in a Maven build world, adding new jars was as simple as editing a BOM. Unneeded jars still had to be edited out of the BOM and if diskspace or clutter was an issue all the jars would be deleted and Maven would refetch those in the BOM. Point though, was that at my level I had no tools that would determine a jar wasn't needed and then just make it go away.

 

[fronobulax]

I fixed CharPaneRequest so that it expects an int for limit mode.

Code behaves before and after my switch to jar.

sequence-library-1.0.4.jar seems not to be needed but I'm not seeing any jar I can relate to jdom. Any chance that the file in your src/jar is an artifact that was not deleted locally when it was deleted from the repository? Alternatively the file name is not intuitive and you need to tell it to me

 

[heeheehee]

Limit mode is not always an int. Per sessions/Limitmode.java, there are cases where we expect the value to be strings like "spelunky" and "batman".

https://sourceforge.net/p/kolmafia/code/HEAD/tree/lib/jar/ contains two files: jdom-2.0.6.jar and sequence-library-1.0.4.jar.

note that I said lib/jar, not src/jar

 

[philmasterplus]

I recall the documentation for HTMLCleaner mentioning JDOM. Perhaps some code is/was using them together?

 

[fronobulax]

I just assumed that limitmode in JSON from charpane was an always an integer. KoLCharacter.setLimitmode() accepts a String, not an int, so I would hope that anywhere KoLmafia wants it to be an integer it is parsing the string.

If charpane can return "different" JSON, then yeah, I broke something that had previously worked.

But AFAIK sessions/Limitmode always calls KoLCharacter.getLimitmode() and always expects a string.

Perhaps the concern is my verbal shorthand? Since I'm headed out feel free to revert until we sort it out.

I got confused since sequence is in both src/jar and lib/jar.

I have deleted it from src/jar with no consequences observed so far.

@philmasterplus may be right in that HTMLCleaner uses them. It would make sense that lib/jar contains dependencies that are need by, but not bundled with, third party code.

 

[heeheehee]

I don't think there's anywhere in Mafia that expects it to be numeric, just that KoL is sometimes giving us a number because reasons. I don't feel any urgency to revert it, as long as it gets done at some point in the next day or so. It's not urgently breaking the build, just impacting Spelunky / Batfellow handling, and if anyone needs it, they can revert to an older revision for a day.

My suspicion is that we needed jdom back when we were bundling the htmlcleaner sources, but that's since moot.

 

[fronobulax]

I don't think there's anywhere in Mafia that expects it to be numeric, just that KoL is sometimes giving us a number because reasons. I don't feel any urgency to revert it, as long as it gets done at some point in the next day or so. It's not urgently breaking the build, just impacting Spelunky / Batfellow handling, and if anyone needs it, they can revert to an older revision for a day.

My suspicion is that we needed jdom back when we were bundling the htmlcleaner sources, but that's since moot.

To clarify, during Spunky and/or Batfellow charpane will provide a String for limitmode whereas at other times charpane will return an integer. The inconsistency is at the KoL end I definitely broke that and I will revert momentarily. The beer will stay cold.

Thanks.