MODPYTHON-93, r387693, backported in r393781, changes the API of
mod_python.util.Field().

I think that it would be a very bad thing to change an API in an
incompatible way in a patch release - whilst people are likely to
understand that things may break going from 3.2.x to 3.3.x, they are
quite likely to expect an upgrade *within* the 3.2.x series to be
relatively painless.

Amongst the applications broken by this, are the 0.9.x current series of
Trac releases.

I suggest un-backporting the API-breaking change from the 3.2.x
maintenance branch.

Max.

Attachment: signature.asc
This has been downloaded 0 time(s).

[ Post a follow-up to this message ]

Max Bowsher wrote:
> MODPYTHON-93, r387693, backported in r393781, changes the API of
> mod_python.util.Field().
>
> I think that it would be a very bad thing to change an API in an
> incompatible way in a patch release - whilst people are likely to
> understand that things may break going from 3.2.x to 3.3.x, they are
> quite likely to expect an upgrade *within* the 3.2.x series to be
> relatively painless.
>
> Amongst the applications broken by this, are the 0.9.x current series of
> Trac releases.
>
> I suggest un-backporting the API-breaking change from the 3.2.x
> maintenance branch.

I agree that we should not make *public* API changes. However quoting
from the docs for util.Field:

"""
4.6.2 Field class

class Field()

This class is used internally by FieldStorage and is not meant to be
instantiated by the user.
"""

The parameters passed in the constructor are not documented so I don't
think users should assume that the api will be invariant. If an
application such as Trac chooses to ignore the documentation they do so
at their own peril.

(That was the bad cop speaking. Now over to the good cop.)

On the other hand, to keep Trac and other such apps from breaking we can
always find a compromise. The changes from 3.2.8 to branches/3.2.x
currently look like this:
@@ -48,19 +48,8 @@
"""

class Field:
-   def __init__(self, name, file, ctype, type_options,
-                disp, disp_options, headers = {}):
+   def __init__(self, name):
self.name = name
-       self.file = file
-       self.type = ctype
-       self.type_options = type_options
-       self.disposition = disp
-       self.disposition_options = disp_options
-       if disp_options.has_key("filename"):
-           self.filename = disp_options["filename"]
-       else:
-           self.filename = None
-       self.headers = headers

def __repr__(self):
"""Return printable representation."""


So what if we back out of this, but re-factor __init__?
-    def __init__(self, name):
+    def __init__(self, name, file=None, ctype=None, type_options=None,
+               disp=None, disp_options=None, headers = {}):


That change should be compatible with the changes in FieldStorage, but
should keep the likes of Trac happy. Ain't Python grand? 

Beyond that, the Trac people should either stop using Field directly, or
advocate that it be designated for public use. I don't personally care
either way, as long as we have a consistent policy. If something in
mod_python is marked for internal use, it really does mean it is for
internal use only.

And of course if the breakage is completely different from what I've
detailed above, feel free to call me an idiot. ;)

Jim

[ Post a follow-up to this message ]

Jim Gallacher wrote:
> Max Bowsher wrote: 
> 
> I agree that we should not make *public* API changes. However quoting
> from the docs for util.Field:
> 
> """
> 4.6.2 Field class
> 
> class Field()
> 
> This class is used internally by FieldStorage and is not meant to be
> instantiated by the user.
> """

Ah, I see. It would probably be good to add a docstring to that effect -
there's no warning not to use it in the code itself - not even an
underscore in its name to hint at it being private.

> The parameters passed in the constructor are not documented so I don't
> think users should assume that the api will be invariant. If an
> application such as Trac chooses to ignore the documentation they do so
> at their own peril.
> 
> (That was the bad cop speaking. Now over to the good cop.)
> 
> On the other hand, to keep Trac and other such apps from breaking we can
> always find a compromise. The changes from 3.2.8 to branches/3.2.x
> currently look like this:
> @@ -48,19 +48,8 @@
>  """
> 
>  class Field:
> -   def __init__(self, name, file, ctype, type_options,
> -                disp, disp_options, headers = {}):
> +   def __init__(self, name):
>         self.name = name
> -       self.file = file
> -       self.type = ctype
> -       self.type_options = type_options
> -       self.disposition = disp
> -       self.disposition_options = disp_options
> -       if disp_options.has_key("filename"):
> -           self.filename = disp_options["filename"]
> -       else:
> -           self.filename = None
> -       self.headers = headers
> 
>     def __repr__(self):
>         """Return printable representation."""
> 
> 
> So what if we back out of this, but re-factor __init__?
> -    def __init__(self, name):
> +    def __init__(self, name, file=None, ctype=None, type_options=None,
> +               disp=None, disp_options=None, headers = {}):
> 
> 
> That change should be compatible with the changes in FieldStorage, but
> should keep the likes of Trac happy. Ain't Python grand? 

That would be a good compromise for 3.2.x, deferring the breakage from a
patch version increment to the next minor version increment.

> Beyond that, the Trac people should either stop using Field directly, or
> advocate that it be designated for public use. I don't personally care
> either way, as long as we have a consistent policy. If something in
> mod_python is marked for internal use, it really does mean it is for
> internal use only.

The use has gone away in Trac 0.10, but that's still working its way
toward release.

> And of course if the breakage is completely different from what I've
> detailed above, feel free to call me an idiot. ;)

No, that's quite correct - the issue is the signature of __init__().

Max.

Attachment: signature.asc
This has been downloaded 0 time(s).

[ Post a follow-up to this message ]