Only skip giveUnitControl if the value is false or true #8023
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8023 +/- ##
============================================
+ Coverage 24.93% 24.95% +0.01%
- Complexity 7328 7332 +4
============================================
Files 1259 1259
Lines 79267 79333 +66
Branches 11162 11159 -3
============================================
+ Hits 19769 19799 +30
- Misses 57402 57438 +36
Partials 2096 2096 Continue to review full report at Codecov.
|
|
If TripleA 2.3 breaks all of the world_war_ii_global games, I would say 2.4 needs to be released as soon as possible. I guess we don't have many pre-release playtesters on these very popular maps (I never play any of them). There is also the problem that any documentation on the matter is missing. It used to be present in some old versions (so, this could be fixed by reverting to one of those). It was even specifically documented in the bottom line of what I linked (as I foresaw this could have been an easy source of confusion for developers and mapmakers).
|
| @@ -895,16 +895,16 @@ private Attachable findAttachment( | |||
| } | |||
| // decapitalize the property name for backwards compatibility | |||
| final String name = LegacyPropertyMapper.mapLegacyOptionName(decapitalize(option.getName())); | |||
| if (LegacyPropertyMapper.ignoreOptionName(name)) { | |||
There was a problem hiding this comment.
Would you mind explaining why this if block is moved down @trevan ?
If we are going to ignore an option, why should we execute option.getValue() or option.getCount(), wouldn't those just be dead stores?
There was a problem hiding this comment.
I moved it because I needed access to option.getValue() and I thought it would make more sense to use the local variable value that was going to be created vs passing in option.getValue(). But I can move it back up and pass in option.getValue().
There was a problem hiding this comment.
The local variable IMO is legacy, it's an unnecessary intermediate variable. This class in general followed a C++ pattern (if not older actually, maybe a C pattern or even Pascal) to declar all local variables at the beginning of a method. IMO all usages of 'value' probably should just get inlined.
I did not spot you were using the value variable. That answers my question.
| static boolean ignoreOptionName(final String name, final String value) { | ||
| return name.equalsIgnoreCase("takeUnitControl") | ||
| || (name.equalsIgnoreCase("giveUnitControl") | ||
| && (value.equals("false") || value.equals("true"))); |
There was a problem hiding this comment.
Are you positive that value could never be null here? I don't remember if we translate empty values to empty strings or null values.
There was a problem hiding this comment.
value is never checked for null in the caller before using (see line 908 in GameParser) so I don't think it would ever be null.
There was a problem hiding this comment.
Considering we just had a NPE in GameParser, I would not presume it to be already correct. Another consideration, the 'value' is very context dependent, a DTD previously would have enforced values being provided, that is not necessarily guaranteed now.
I wonder if there is any type of test coverage around this at all that could/should be updated.
There was a problem hiding this comment.
Hmm, I may be wrong about the DTD previously requiring a specific value, I'm not sure if it can constrain attribute values vs just requiring them being there. Regardless, it is context dependent if we can expect a value.
There was a problem hiding this comment.
Good point on the NPE. It will now skip giveUnitControl if the value is null as well as true or false.
There is some test coverage but it requires creating a brand new xml with these attributes in it. I'm looking at if there is a way to be able to create the xml in memory instead of committing the xml as a resource.
There was a problem hiding this comment.
If we can get away from XMLs for testing, that would be excellent.
The decoupling of XML parsing from the semantic data parsing into GameData was meant to help with this.
One should be able to set up a org.triplea.map.data.elements.Game object and then pass that to GameParser and avoid having to set up an XML. The former is well tested for a variety of XML snippets, so we can be pretty sure how that works. It should have some existing tests that might be interesting to know if we null out empty values. Beyond that, sending GameParser a defined Game object hopefully can do the trick. GameParser is still very rough, so it might take some plumbing work to accept a Game object here. Let me know if that turns out to be the case, for now a manual verification is enough or you can go the extra mile if you feel it's worthwhile given the context acquisition cost you will have payed.
|
@DanVanAtta , I responded to your comments |
Fixes #7990
In #1033, @Cernelius said to remove
giveUnitControlif its values was a boolean. But he said to leave it alone if the value wasn't a boolean. In #7631, it was just skipped without looking at the value. This changes it to check the value and only skip it if it is a boolean.I think this breaks all of the world_war_ii_global games as well as several others.
Testing
Played World War 2 Global 1940 2nd to UK_Pacific Place round, placed some units in India, finished the place round and checked that the new units are now British units.
Screens Shots
Additional Notes to Reviewer
Release Note
FIX|World War 2 Global will correctly switch unit ownership of units placed in India from UK_Pacific to British.