Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
May 13, 2024

Conversation

miscoined
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

ebullient commented May 12, 2024

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)

@ebullient ebullient force-pushed the main branch 3 times, most recently from 35cde4d to ee51422 Compare May 13, 2024 12:54
…se them everywhere

🐛 Fix a bug for spells where a range of eg 50 would get represented as "50 foot" instead of "50 feet"
🐛 Fix a bug where Immunities/Resistances/Weaknesses would get listed in the wrong order - this is probably due to a previous change I made, where I didn't think of Map.of not preserving the insertion order
🐛 Fix a bug where HpHardnessBt was formatted incorrectly with spaces before semicolons when there were no associated notes

This is a pretty far-reaching change, so I tested it by doing the following:
1. Ran tests while checked out to upstream/main
2. Move target/test-pf2 to target/test-base-pf2e
3. Ran tests while checked out to this branch (with the changes for the two above bugs reverted)
4. Ran `diff -BburZ test-base-pf2 test-pf2` to compare differences
@miscoined
Copy link
Collaborator Author

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 🤦

@ebullient ebullient merged commit 17cb577 into ebullient:main May 13, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants