Partial data reading implementation

1. General comments

1. You have a tendency to over-complicate things. As a result, your code is full of mistakes that you failed to spot. Keep code as simple as possible and it will be easier to inspect and test!

2. The code you posted is missing a bunch of import statments needed to run. To help other reviewers, I think that these imports need to be:

   import base64
   import gzip
   import os
   import unittest
   import xml.etree.ElementTree as ET
   import zlib
   

3. It is worth keeping to 79 columns as recommended by the Python style guide (PEP8). Your code is hard to read here on Code Review because the long lines don't fit and we have to scroll horizontally to read them.

2. value_equality decorator

1. Your decorator value_equality does not work! It looks as though you have not tested it in the subclass case. Suppose we have:

   @value_equality
   class A: pass
   class B(A): pass
   

   then an object of the subclass B does not compare equal to an object of the superclass A if we use the == operator:

   >>> a, b = A(), B()
   >>> a == b
   False
   

   and yet the __eq__ method works fine:

   >>> a.__eq__(b)
   True
   

   The Python feature that explains this behaviour is a bit obscure, but it is documented, in this note about the implementation of special methods:

   Note: If the right operand’s type is a subclass of the left operand’s type and that subclass provides the reflected method for the operation, this method will be called before the left operand’s non-reflected method. This behavior allows subclasses to override their ancestors’ operations.

   Combined with the fact that __eq__ is its own reflected operation, this means that when Python evaluates a == b, it notes that the type of b is a subclass of the type of a, and so it calls b.__eq__(a), which tests isinstance(a, b.__class__) which is False.

   This highlights a basic problem with the design of value_equality. Equality should be a symmetric operation: that is, if a equals b then b should equal a, and so it shouldn't matter which way round the comparison is done. But in your implementation you use an isinstance test which is not symmetrical.

2. I like that you've written docstrings. But unfortunately you've written them from the wrong point of view. The purpose of a docstring is to provide help to the user: that is, to answer questions like "what does this do?" and "how do I use it?". For example:

   >>> help(abs)
   Help on built-in function abs in module builtins:
   
   abs(...)
       abs(number) -> number
   
       Return the absolute value of the argument.
   

   But you've written your docstrings from the point of view of the implementor: basically they are notes to yourself that really should be comments instead. For example, the docstring for value_equality is

   Implement __eq__ and __ne__; caution, does not work with polymorphic yet.
   

   where the first clause is a detail of the implementation, and the second is just obscure to me. (See below for how the docstring should look.)

   Similarly, the docstring for the Data class contains a description of how the list of tiles is encoded into a <data> element. This is handy for you as an implementor, but is useless for the user of the Data class. Put this information in a comment (or better still, provide a link to the TMX documentation, which is more comprehensive).

3. value_equality doesn't need to be a decorator because it does not look at the implementation of cls. So you should prefer to make it into a class instead. Classes are simpler to understand than decorators.

   class EqualAttributes(object):
       """Class of objects that can be compared for equality. Instances
       compare equal if their type and writable attributes are equal. For
       example:
   
           >>> class Foo(EqualAttributes): pass
           >>> a, b = Foo(), Foo()
           >>> a.x, b.x = 1, 1
           >>> a == b, a != b
           (True, False)
           >>> a.y, b.y = 1, 2
           >>> a == b, a != b
           (False, True)
   
       """
       def __eq__(self, other):
           return type(self) == type(other) and self.__dict__ == other.__dict__
   
       def __ne__(self, other):
           return not (self == other)
   

   Notes:

   • The docstring is written from the user's point of view.
   • There are some examples in the form of doctests.
   • I've used type(self) which is clearer than self.__class__.
   • I've addressed the symmetry problem by restricting equality to objects of the same type.

4. The Data and DataTile classes have __eq__ and __ne__ methods, so why do they also have @value_equality? Surely they should have one or the other, but not both?

5. Are you sure you really need the value_equality decorator at all? You only ever use it in your unit tests, so I think things would be much simpler if you just got rid of this class and compared attributes directly in the test cases.

3. DataTile class

1. There's no docstring for the DataTile class. What does this class do and how should I use it?

2. The name DataTile seems superfluous. Why not just Tile?

3. Instead of:

   HORIZONTAL_FLIP_BIT = 0x80000000
   VERTICAL_FLIP_BIT   = 0x40000000
   DIAGONAL_FLIP_BIT   = 0x20000000
   

   I would write:

   HORIZONTAL_FLIP = 1 << 31
   VERTICAL_FLIP   = 1 << 30
   DIAGONAL_FLIP   = 1 << 29
   

   to make it clear where these numbers come from.

4. The from_bytes method is mis-named: it doesn't actually take a sequence of bytes as an argument, but rather a tile id as a number.

5. In the from_bytes method, it's pointless to build a dictionary and then immediately pass it as keyword arguments. Just pass the keyword arguments instead.

6. The value ~(cls.HORIZONTAL_FLIP_BIT | cls.VERTICAL_FLIP_BIT | cls.DIAGONAL_FLIP_BIT) is always the same, so why not compute it just once?

7. There doesn't seem to be any need for separate __init__ and from_bytes methods. You only use __init__ in the test cases. So I would replace __init__ with from_bytes and adjust the test cases accordingly.

4. Data class

1. It's not clear to me what the purpose of this class is. What kind of thing does a Data object represent? As far as I can see, the only purpose of this class is to produce a list of DataTile objects from a <data> element. So why not just write a function?

2. The properties Data.encoding and Data.compression are buggy: you need to explicitly return the value.

3. But why bother with properties at all? In Python, properties are normally used in three scenarios. First, to provide a simple interface to a complex data structure. Second, to give the implementer freedom to change the implementation in future. Third, in combination with setters when writing a public API whose internal data structures have an invariant that needs to be preserved. But it's not clear to me that any of those scenarios apply to your code, so why not keep it simple and just use attributes?

4. @staticmethod is usually an anti-pattern in Python. When code has nothing to do with a class, then there's no need to make it a static method, you can just make it a plain function instead. Python is not Java (not everything needs to be part of a class).

5. It's not clear why Data.__init__ takes compression or encoding values: these are never used.

6. There seems to be some confusion in this class about global_tile_ids versus tiles.

7. Data.__init__ takes a parameter with a default value of an empty list: global_tile_ids=[]. Default values for function parameters are evaluated only once, when the function is defined. So all instances of Data will share the same default value for global_tile_ids.

   If this isn't what you want, you should create a new empty list for each instance:

   def __init__(..., global_tile_ids=None, ...):
       # ...
       self.global_tile_ids = global_tile_ids or []
   

8. Instead of:

   element.attrib['encoding'] if 'encoding' in element.attrib else None
   

   use the dict.get method and write:

   element.attrib.get('encoding')
   

9. This code doesn't work:

   map(tile_ids.append, element.iter('tile'))
   

   There are two problems. First, map returns an iterator and does not actually apply the function until you iterate over it. Second, even if you do iterate, you'll get a list of <tile> elements, whereas what you want is a list of global ids.

   So write instead:

   tile_ids = [tile.attrib['gid'] for tile in element.iter('tile')]
   

10. This:

    lambda x: gzip.decompress(data)
    

    looks like a mistake for:

    lambda x: gzip.decompress(x)
    

    but this is just the same as:

    gzip.decompress
    

    (which computer scientists call η-equivalence).

11. I think you've misunderstood the TMX specification. Or at least, let us say, the TMX specification is very unclear. You've written, "Data is compressed first, then encoded" but that can't be right when encoding=csv because the types don't match. Compression outputs a byte stream, but CSV encoding takes a list of gids as input. These don't match, so the combination can't make any sense.

    The only way that I can see to make sense of the specification is that there are only four possibilities:

    compression  encoding  gids represented as
    -----------  --------  -----------------------------------------------
    None         None      gid= attributes in <tile> elements
    None         'csv'     comma-separated numbers, encoded in ASCII
    'gzip'       'base64'  32-bit numbers, gzip-compressed, base64-encoded
    'zlib'       'base64'  32-bit numbers, zlib-compressed, base64-encoded
    

    It might be worth opening an issue on GitHub and see if Thorbjørn Lindeijer can clarify the documentation.

12. You don't properly decode the tile data! After decompressing it you just call

    tile_ids = list(decompressed)
    

    but this results in tile_ids being a list with one element for each byte in the decompressed data. But in the TMX documentation it says that these bytes "should be interpreted as an array of unsigned 32-bit integers using little-endian byte ordering." You have completely omitted this step.

5. Revised code

import base64
import gzip
import struct
import zlib

class Tile(object):
    """An instance of a tile in a map."""

    HORIZONTAL_FLIP = 1 << 31
    VERTICAL_FLIP   = 1 << 30
    DIAGONAL_FLIP   = 1 << 29
    TILE_ID_MASK    = ~(HORIZONTAL_FLIP + VERTICAL_FLIP + DIAGONAL_FLIP)

    def __init__(self, gid):
        self.global_tile_id = gid & self.TILE_ID_MASK
        self.flipped_horizontally = bool(gid & self.HORIZONTAL_FLIP)
        self.flipped_vertically = bool(gid & self.VERTICAL_FLIP)
        self.flipped_diagonally = bool(gid & self.DIAGONAL_FLIP)

def parse_data(element):
    """Parse a <data> element from Tiled Map Format (TMX) and return a
    list of Tile objects.

    """
    assert(element.tag == 'data')
    encoding = element.attrib.get('encoding')
    compression = element.attrib.get('compression')
    if encoding not in (None, 'base64', 'csv'):
        raise ValueError('unsupported encoding={}'.format(encoding))
    if compression not in (None, 'gzip', 'zlib'):
        raise ValueError('unsupported compression={}'.format(compression))
    if not encoding and not compression:
        return [Tile(int(tile.attrib['gid'])) for tile in element.iter('tile')]
    elif encoding == 'csv' and not compression:
        gids = [int(gid) for gid in data.split(',')]
    elif encoding == 'base64':
        data = base64.b64decode(element.text)
        if compression == 'gzip':
            data = gzip.decompress(data)
        elif compression == 'zlib':
            data = zlib.decompress(data)
        gids = struct.unpack('<{}L'.format(len(data) // 4), data)
    else:
        raise ValueError('unsupported combination: encoding={} compression={}'
                         .format(encoding, compression))
    return [Tile(gid) for gid in gids]