Increases the coverage of codecs.py by about 3 lines...

Also some pep8 compliance outside of the scope of the added lines of code.

Tennessee, there were a few issues in the original patch - see the attached update (mostly the use of assert statements instead of methods and the overbroad scope of the context manager when checking for an expected exception).

However, in getting ready to commit this, I noticed something interesting. The docs for the IncrementalEncoder.getstate() API [1] state that the value returned *must* be an integer.

BufferedIncrementalEncoder doesn't obey that limitation - when data is buffered, it returns the raw buffered string instead of encoding it as some kind of integer.

As a separate, but related point, IncrementalDecoder.getstate() includes an explanation on how to save arbitrary state as an integer, but no such explanation (not even a reference to the IncrementalDecoder version) is present in the IncrementalEncoder.getstate() docs.

Adding MAL, since I'd like an expert opinion. Is the API less stringent than the docs state, or should BufferedIncrementalEncoder be fixed to always return the state as an integer?

[1] http://docs.python.org/dev/library/codecs#codecs.IncrementalEncoder.getstate

Nick Coghlan wrote:
> 
> As a separate, but related point, IncrementalDecoder.getstate() includes an explanation on how to save arbitrary state as an integer, but no such explanation (not even a reference to the IncrementalDecoder version) is present in the IncrementalEncoder.getstate() docs.
> 
> Adding MAL, since I'd like an expert opinion. Is the API less stringent than the docs state, or should BufferedIncrementalEncoder be fixed to always return the state as an integer?
> 
> [1] http://docs.python.org/dev/library/codecs#codecs.IncrementalEncoder.getstate

I'm not sure how that description got into the docs. It must
have been added in the 3.x branch, since the 2.7 version of the
docs doesn't document those method at all.

FWIW: The .getstate() and .setstate() don't restrict the type of data
used for storing the state. The only requirement is that the data
returned by .getstate() can be passed to .setstate() in order to
recreate the internal state used by the codec:

    def getstate(self):
        """
        Return the current state of the encoder.
        """
        return 0

    def setstate(self, state):
        """
        Set the current state of the encoder. state must have been
        returned by getstate().
        """
For practical reasons, the used data should be pickleable.

The interface is very similar to the __getstate__/__setstate__
interface used by pickle.

Some more tests, updated initial state of BufferedIncrementalEncoder to be the correct type, updated rst file. Bit tired, hope I got it right!

Thanks for the feedback everyone, helps me to get it done, even if it's more work for you...

Tennessee Leeuwenburg wrote:
> 
> Tennessee Leeuwenburg <tleeuwenburg@gmail.com> added the comment:
> 
> Some more tests, updated initial state of BufferedIncrementalEncoder to be the correct type, updated rst file. Bit tired, hope I got it right!
> 
> Thanks for the feedback everyone, helps me to get it done, even if it's more work for you...

I think you should simply drop this whole part:

"""
The implementation should make sure that ``0`` is the most common state. (States
that are more complicated than integers can be converted into an integer by
marshaling/pickling the state and encoding the bytes of the resulting string
into an integer).
"""

or, better, rephrase it so that it becomes clear that the codec
implementation may use any type of pickleable object to represent
the internal state. The only requirement is that .setstate()
has to be able to read back the state returned by .getstate().

We have codecs in the stdlib that return integers, bytes/string
and even tuples (see the io module and the UTF-16 codec as example),
so the documentation is clearly wrong.

Thanks,
-- 
Marc-Andre Lemburg
eGenix.com

________________________________________________________________________
2011-10-04: PyCon DE 2011, Leipzig, Germany                43 days to go

::: Try our new mxODBC.Connect Python Database Interface for free ! ::::

   eGenix.com Software, Skills and Services GmbH  Pastor-Loeh-Str.48
    D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg
           Registered at Amtsgericht Duesseldorf: HRB 46611
               http://www.egenix.com/company/contact/

Thanks for the review. Here is a patch incorporating the two comments being to move some comments.

Tennessee Leeuwenburg wrote:
> 
> Tennessee Leeuwenburg <tleeuwenburg@gmail.com> added the comment:
> 
> Thanks for the review. Here is a patch incorporating the two comments being to move some comments.

Hmm, the documentation patch doesn't appear to have changed. Did you
upload the right patch ?

Here is a stab at updated documentation. I would suggest that if further changes are recommended to the documentation, that a core committer go ahead and make them. I'm absolutely more than happy to keep taking "stabs" at it, but ultimately I probably don't understand the purpose of these classes as well as some of the rest of you, and I don't feel best placed to decide exactly how this should read....

Would someone who's commented previously do a commit review please?

The requirement that getstate() returns a (buffer, int) tuple has to do with the fact that for text streams seek() and tell() somehow have to take the state of the codec into account. See _pyio.TextIOWrapper.(seek|tell|_pack_cookie|_unpack_cookie).

However I can't remember the exact history of the specification.

The patch would have to be updated to 3.5 (part of it no longer applies), but other than that I think it's fine.

It may make sense to readd the comment for .getstate() to keep the state as simple as possible ("The implementation should make sure that ``0`` is the most common state."), but without requiring a specific number, type, etc.

See also issue20420.