-
Notifications
You must be signed in to change notification settings - Fork 1
Fix EntityStore NPE, Connecting to HPGTracker, and cleanup #128
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
Conversation
| </planet> | ||
| <planet> | ||
| <name>Victoria</name> | ||
| <name>Victoria (Meglan)</name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice research-based fix - this was a renamed planet according to Sarna, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sarna calls it Victoria II IIRC, "Victoria (Meglan)" is what megamek/mm-data calls it.
| } else { | ||
| pFile = new File("./campaign/players/" + name.toLowerCase() + ".dat"); | ||
| } | ||
| File pFile = new File("./campaign/players/" + name.toLowerCase() + ".dat"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it was technically possible to have two different save files for a single player, now there's only the one... any unintended consequences? I don't think so but just for extra consideration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have left a comment explaining this. There shouldn't be any regressions, the only place this method got called was https://github.com/Raugharr/MekWars/pull/128/changes#diff-a021e721cb45661b5d3cd0838cb5bd7f27e987a268bdb6e261eac0916cf3c0fbL1154 that hard coded the parameter to false.
| @Override | ||
| public String toString() | ||
| { | ||
| String result = "CON$"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringBuilder is more performant, but perhaps this is more readable?
sandysimonton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few things for consideration, looks good!
3228b20 to
0017422
Compare
No description provided.