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

Simplify map data #7233

Merged
merged 2 commits into from
Jul 24, 2020
Merged

Simplify map data #7233

merged 2 commits into from
Jul 24, 2020

Conversation

DanVanAtta
Copy link
Member

@DanVanAtta DanVanAtta commented Jul 24, 2020

2 Commits to simplify, please verify that the logic & changes are sound:

commit c645e41

Simplify map parameter, pass keyset instead as we only use the map keys.

commit 299c68f

Remove 'ignore missing territories block'

This block does not do anything.. We later check if all map
territories are in the keyset that comes from a save game. If
we have extra territories in the save game, removing them is
not going to have an effect when we check that all territories
in the map are in the save game.

IE:

The following contains check will be no different if we decrease the size
of the 'keys' set:
```
      if (!keys.contains(terr.getName())) {
```

Functional Changes

[] New map or map update
[] New Feature
[] Feature update or enhancement
[] Feature Removal
[x] Code Cleanup or refactor
[] Configuration Change
[] Problem fix
[] Other:

Testing

  • smoke test: checked that any map still loads on single player

Screens Shots

Additional Notes to Reviewer

It looks like the iter remove may have been a wrong fix to a previous problem: 0eaf35b , notably the whole iter.remove could have been removed.

Release Note

This block does not do anything.. We later check if all map
territories are in the keyset that comes from a save game. If
we have extra territories in the save game, removing them is
not going to have an effect when we check that all territories
in the map are in the save game.

IE:

The following contains check will be no different if we decrease the size
of the 'keys' set:
```
      if (!keys.contains(terr.getName())) {
```

It appears an error used to be logged here instead of the 'skip'. It seems
that instead of skipping the iter.remove and surrounding loop could have
been removed in 0eaf35b
@codecov
Copy link

codecov bot commented Jul 24, 2020

Codecov Report

Merging #7233 into master will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #7233   +/-   ##
=========================================
  Coverage     23.82%   23.83%           
+ Complexity     6970     6969    -1     
=========================================
  Files          1160     1160           
  Lines         78793    78785    -8     
  Branches      11370    11368    -2     
=========================================
- Hits          18776    18775    -1     
+ Misses        57836    57828    -8     
- Partials       2181     2182    +1     
Impacted Files Coverage Δ Complexity Δ
...ava/games/strategy/triplea/ui/mapdata/MapData.java 2.78% <0.00%> (+0.07%) 3.00 <0.00> (ø)
.../java/games/strategy/triplea/delegate/Matches.java 42.91% <0.00%> (-0.11%) 357.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78f1562...f2a94f2. Read the comment docs.

@codeclimate
Copy link

codeclimate bot commented Jul 24, 2020

Code Climate has analyzed commit f2a94f2 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 26.6% (0.0% change).

View more on Code Climate.

@RoiEXLab RoiEXLab merged commit 83da4b0 into master Jul 24, 2020
@RoiEXLab RoiEXLab deleted the simplify-map-data branch July 24, 2020 10:13
DanVanAtta added a commit that referenced this pull request Aug 22, 2020
Allows for new territory naming to be ignored in favor of "old"
territory. This allows territory naming to line up with polygons.txt
and centers.txt, without the matching is strict and territory names
can appear as 'none' which disallows movements into those territories.

Partial revert of: #7233
Fixes: #7386
DanVanAtta added a commit that referenced this pull request Aug 22, 2020
Allows for new territory naming to be ignored in favor of "old"
territory. This allows territory naming to line up with polygons.txt
and centers.txt, without the matching is strict and territory names
can appear as 'none' which disallows movements into those territories.

Partial revert of: #7233
Fixes: #7386
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