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.
signature.asc
Description: OpenPGP digital signature
