ebullient / ttrpg-convert-cli

Utility to convert JSON data (for content you own) from 5etools or pf2etools into Obsidian-friendly Markdown.
https://www.ebullient.dev/projects/ttrpg-convert-cli/
Apache License 2.0
184 stars 37 forks source link

Add a stringutil helper and incidentally fix some string-related bugs #448

Closed miscoined closed 4 months ago

miscoined commented 4 months ago

I've added a StringUtil helper with what I hope is a very clear scope - only string-related methods, and only methods which don't require any domain-specific knowledge. I've also changed some of the previous code I've written (and adjacent) to use these helpers. This actually incidentally resulted in a few fixed bugs (see second commit).

The first commit is mostly just IntelliJ refactors, moving existing methods into StringUtil. The second commit adds some new helpers and modifies some of the behavior of the existing helpers to make them more widely applicable. I know this is a bit far reaching, so for the second commit I've done an extra testing step of diffing the before/after changes (see that commit message for details), and aside from the fixed bugs there aren't any output changes.

Sorry if I went a bit overboard with the Collector 😅. I just really like streams.

ebullient commented 4 months ago

I think it's interesting that you're using records. I haven't had time to fuss with those much. I couldn't decide if pulling them in here was worth it or not, so I didn't. ;)

miscoined commented 4 months ago

I think it's interesting that you're using records. I haven't had time to fuss with those much. I couldn't decide if pulling them in here was worth it or not, so I didn't. ;)

Mostly I see them as a useful way to reduce boilerplate. The benefits are reduced here because generally instance variables have public access, but I see it as still worth it to avoid having to essentially repeat your instance variables three times (once for the declaration, once for the constructor arg, and once to assign it). Even though the IDE can automate away the writing of it, it's still clutter, although it does mean giving up mutable variables.

I'm a big Kotlin fan, which probably shows through. It's hard to go without all of the Kotlin niceties once you get used to them (extension methods, my beloved...)

ebullient commented 4 months ago

I'm a big Kotlin fan, which probably shows through. It's hard to go without all of the Kotlin niceties once you get used to them (extension methods, my beloved...)

Yea.. a good friend of mine is all about Kotlin. I can't get there from here either (yet). I learned Rust instead.. :-P

ebullient commented 4 months ago

I think it's interesting that you're using records. I haven't had time to fuss with those much. I couldn't decide if pulling them in here was worth it or not, so I didn't. ;)

Mostly I see them as a useful way to reduce boilerplate. The benefits are reduced here because generally instance variables have public access, but I see it as still worth it to avoid having to essentially repeat your instance variables three times (once for the declaration, once for the constructor arg, and once to assign it). Even though the IDE can automate away the writing of it, it's still clutter, although it does mean giving up mutable variables.

You aren't wrong here, especially as I'm essentially using them as records. I just wasn't sure how Qute and Jackson and all the other things would work with them, and didn't want to spend that time figuring out. I was wrestling enough trolls dealing with the data. ;)

ebullient commented 4 months ago

Sorry if I went a bit overboard with the Collector 😅. I just really like streams.

I do avoid streams in some cases... i.e. there are methods to iterate over Jackson JsonObject fields or JsonArray elements. Most of the time, I use regular for loops for these because when shit goes wrong, debugging the stream form is harder than it has to be (and the stack traces are extra stupid). Outside of that, I don't have an issue with streams. ;)

ebullient commented 4 months ago

We hit an issue on the 5e side:

 java.lang.NumberFormatException: For input string: "30 tons"
    at java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:67)
    at java.base/java.lang.Integer.parseInt(Integer.java:668)
    at java.base/java.lang.Integer.parseInt(Integer.java:786)
    at dev.ebullient.convert.StringUtil.pluralize(StringUtil.java:218)
    at dev.ebullient.convert.tools.dnd5e.qute.QuteVehicle$ShipCrewCargoPace.toString(QuteVehicle.java:179)
miscoined commented 4 months ago

Fixed, I think. I've since realized that while I was running the DnD 5e tests, I'd neglected to actually add any source data - so the tests weren't actually doing anything 🤦