Minimum Product: World map ported to python3. #254
Conversation
OlafSzmidt
requested review from
Spycho,
mrniket,
ramonfmir,
danalex97 and
CelineBoudier
|
@danalex97 There are hundreds of places where the 2to3 move needs to done, not just world_map. Small steps. Will the codebase work with py2 dockerfiles for now? I wanted this PR to just be for world_map functionality :) |
|
@OlafSzmidt Oh, I see. That's why you did the itervalues to work with both versions. That makes sense. |
|
@danalex97 Exactly ;) And itervalues() are not compatible with py3, but values() are back-compatible with py2 because they just return a list. I'm just pushing an extra test for that functionality if you could check that before you go :P |
OlafSzmidt
force-pushed
the
world_map_to_py3
branch
from
1e7d3dd to
3f3b781
Compare
|
@danalex97 To be honest I was hoping that the Seems to pass; therefore I guess it's a problem with the coverage tool (or else I'm misunderstanding the use of |
| @@ -118,6 +120,22 @@ def test_all_cells(self): | |||
| self.assertIn('D', cell_names) | |||
| self.assertEqual(len(cell_names), 4) | |||
|
|
|||
| def test_all_cells_python3(self): | |||
| # Except block will be called when python3 is used. | |||
| with self.assertRaises(Exception) as e: | |||
There was a problem hiding this comment.
I think this test is wrong. This checks that an exception has been raised. The exception raised is in these lines:
self.assertFalse(isinstance(cell_names, collections.Iterable))
self.assertTrue(isinstance(cell_names, list))
since the assertion is false in python2. You can check this by commenting these lines. Thus, the test passes in python2. In python3, the test would fail since no exception would have been raised.
I think we should have only one test, that should work regardless of the python version or no test at all.
There was a problem hiding this comment.
The previous test will pass on both versions of python; and I suppose test coverage isn't THAT important in this case. I'll get rid of it. Thanks! 👍
There was a problem hiding this comment.
👍 I'm curious if that is something actually testable and how one would do that.
| @@ -305,7 +323,7 @@ class TestWorldMapWithOriginCentre(TestWorldMap): | |||
| def _generate_grid(self, columns=2, rows=2): | |||
| alphabet = iter(ascii_uppercase) | |||
| grid = {Location(x, y): MockCell(Location(x, y), name=next(alphabet)) | |||
| for x in xrange(-int_ceil(columns/2.0)+1, int_floor(columns/2.0)+1) for y in xrange(-int_ceil(rows/2.0)+1, int_floor(rows/2.0)+1)} | |||
| for x in range(-int_ceil(columns/2.0)+1, int_floor(columns/2.0)+1) for y in range(-int_ceil(rows/2.0)+1, int_floor(rows/2.0)+1)} | |||
There was a problem hiding this comment.
Didn't notice. Good catch with those 👍
OlafSzmidt
force-pushed
the
world_map_to_py3
branch
from
69152be to
b45f0a9
Compare
|
Thanks @danalex97. @CelineBoudier @ramonfmir @mrniket - any remarks before I merge? |
aimmo-game/simulation/world_map.py
Outdated
| ''' | ||
| try: | ||
| values = self.grid.itervalues() | ||
| except: |
There was a problem hiding this comment.
you could try to except for a specific error type, less messy - in case the problem is not linked to the version of python for instance?
|
but maybe squash before |
New test for all_cells in py3. Quickfix for test_all_cells test Unnecessary new test removed. Exception specified for all_cells.
OlafSzmidt
force-pushed
the
world_map_to_py3
branch
from
63b43c2 to
26d89ef
Compare
|
Squashed. |
Ported
world_mapandtest_world_mapto python3, ie:xrangeis now replaced isrange.tryandexceptused in instances whereitervalues()is used. This way I can allow for both versions to run the code.