Partial data reading implementation

I'm trying to learn Python and am working on a simple Tiled Map Format (.tmx) reader as practice. So as to not post too much code at once, I'm only publishing the ` element which stores the tiles displayed in a tiled map.

I'm looking for any critique, no matter how harsh, on quality of code, the Pythonic way of coding, my unit tests, etc.

First, the description of the element from the documentation:

encoding: The encoding used to encode the tile layer data. 
          When used, it can be "base64" and "csv" at the moment.
compression: The compression used to compress the tile layer data. 
             Supports "gzip" and "zlib".

First you need to base64-decode it, then you may need to decompress it. 
Now you have an array of bytes, which should be interpreted as an array of 
unsigned 32-bit integers using little-endian byte ordering.

Whatever format you choose for your layer data, 
you will always end up with so called "global tile IDs" (gids).

The code:

``
@value_equality
class Data:
"""
Contains the information on how the current map is put together.

Data is compressed first, then encoded. Then the data is an array of bytes which should be interpreted
as an array of unsigned 32-bit integers in little-endian format.

If neither encoding nor compression is used, then the data is stored as an xml list of tile elements.

All formats represent global tile ids and may refer to any tilesets used by the map. In order to correctly map the
global id to the tileset, find the tileset with the highest firstgid that is still lower or equal than the gid. The
tilesets are always stored with increasing firstgids
"""

@property
def encoding(self):
"""
The encoding used to encode the tile layer data.
Can be either "base64" or "csv".
(Optional)
"""
self._encoding

@property
def compression(self):
"""
The compression used to compress the tile layer

1. General comments

-
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!

-
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

-
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.

1. value_equality decorator

-
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.

-
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 ` 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).

-
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.

-
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?

-
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.

1. DataTile class

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

-
The name DataTile` seems super

StackExchange Code Review Q#38246, answer score: 3