quetz-mod_python-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Max Bowsher <ma...@ukf.net>
Subject Re: 3.2.9 release blocker? API breakage for mod_python.util.Field()
Date Thu, 22 Jun 2006 21:32:28 GMT
Jim Gallacher wrote:
> 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.
> """

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.


Mime
View raw message