Discussion:
[Twisted-Python] incompatible change - need revert before release
Glyph Lefkowitz
2015-11-20 05:06:30 UTC
Permalink
Tom Prince discovered a regression on https://twistedmatrix.com/trac/ticket/7016#comment:14 <https://twistedmatrix.com/trac/ticket/7016#comment:14> - I think that this was introduced after 15.4, so it needs to be rolled back (or fixed, if someone can get to it before the revert) in 15.5.

-glyph
Adi Roiban
2015-11-20 08:08:27 UTC
Permalink
It was released before 15.4

Twisted Web 15.2.0 (2015-05-18)
===============================

Features
--------
- twisted.web.server.Site accepts requestFactory as constructor
argument. (#7016)

----------

Not sure if rollback is the right thing to do... but I have no idea how to
proceed as any change will back the compatibility.

I guess that we should just create a normal bug ticket and fix this issue


Regards,
Adi
Post by Glyph Lefkowitz
Tom Prince discovered a regression on
https://twistedmatrix.com/trac/ticket/7016#comment:14 - I think that this
was introduced after 15.4, so it needs to be rolled back (or fixed, if
someone can get to it before the revert) in 15.5.
-glyph
_______________________________________________
Twisted-Python mailing list
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
--
Adi Roiban
Glyph Lefkowitz
2015-11-20 08:32:39 UTC
Permalink
Post by Adi Roiban
It was released before 15.4
Twisted Web 15.2.0 (2015-05-18)
===============================
Features
--------
- twisted.web.server.Site accepts requestFactory as constructor
argument. (#7016)
----------
Not sure if rollback is the right thing to do... but I have no idea how to proceed as any change will back the compatibility.
I guess that we should just create a normal bug ticket and fix this issue
Thanks for finding the changelog entry; sorry for the false alarm.

If it's been in a release, then there's probably nothing to do. It's a shame that this went out, but once a breakage like this has happened we have to live with it because otherwise, as you say, we'd be breaking compatibility for the people that already upgraded. For those that need to support both versions, keyword arguments are the way to go.

-glyph
Adi Roiban
2015-11-20 09:42:36 UTC
Permalink
Post by Adi Roiban
Post by Adi Roiban
It was released before 15.4
Twisted Web 15.2.0 (2015-05-18)
===============================
Features
--------
- twisted.web.server.Site accepts requestFactory as constructor
argument. (#7016)
----------
Not sure if rollback is the right thing to do... but I have no idea how
to proceed as any change will back the compatibility.
Post by Adi Roiban
I guess that we should just create a normal bug ticket and fix this issue
Thanks for finding the changelog entry; sorry for the false alarm.
If it's been in a release, then there's probably nothing to do. It's a
shame that this went out, but once a breakage like this has happened we
have to live with it because otherwise, as you say, we'd be breaking
compatibility for the people that already upgraded. For those that need to
support both versions, keyword arguments are the way to go.
Well, in public interfaces we could just stop mixing *args and **kwargs
with other arguments.

It is more work for maintainers, but as a library user I find it much
easier to see the exact args in the docs, rather than seeing *args /
**kwargs and then navigating the inheritance path to find out all supported
arguments.
--
Adi Roiban
Glyph Lefkowitz
2015-11-20 10:25:14 UTC
Permalink
Post by Glyph Lefkowitz
Post by Adi Roiban
It was released before 15.4
Twisted Web 15.2.0 (2015-05-18)
===============================
Features
--------
- twisted.web.server.Site accepts requestFactory as constructor
argument. (#7016)
----------
Not sure if rollback is the right thing to do... but I have no idea how to proceed as any change will back the compatibility.
I guess that we should just create a normal bug ticket and fix this issue
Thanks for finding the changelog entry; sorry for the false alarm.
If it's been in a release, then there's probably nothing to do. It's a shame that this went out, but once a breakage like this has happened we have to live with it because otherwise, as you say, we'd be breaking compatibility for the people that already upgraded. For those that need to support both versions, keyword arguments are the way to go.
Well, in public interfaces we could just stop mixing *args and **kwargs with other arguments.
It is more work for maintainers, but as a library user I find it much easier to see the exact args in the docs, rather than seeing *args / **kwargs and then navigating the inheritance path to find out all supported arguments.
I'm not quite sure what you're referring to in this case; but generally, I agree. If you pass a parameter, you should document it with @param even if your arg list says *args / **kwargs. This is how I tried to document, for example, optionsForClientTLS.

-glyph
Adi Roiban
2015-11-20 10:47:12 UTC
Permalink
Post by Adi Roiban
Post by Adi Roiban
Post by Adi Roiban
It was released before 15.4
Twisted Web 15.2.0 (2015-05-18)
===============================
Features
--------
- twisted.web.server.Site accepts requestFactory as constructor
argument. (#7016)
----------
Not sure if rollback is the right thing to do... but I have no idea how
to proceed as any change will back the compatibility.
Post by Adi Roiban
I guess that we should just create a normal bug ticket and fix this
issue
Thanks for finding the changelog entry; sorry for the false alarm.
If it's been in a release, then there's probably nothing to do. It's a
shame that this went out, but once a breakage like this has happened we
have to live with it because otherwise, as you say, we'd be breaking
compatibility for the people that already upgraded. For those that need to
support both versions, keyword arguments are the way to go.
Well, in public interfaces we could just stop mixing *args and **kwargs
with other arguments.
It is more work for maintainers, but as a library user I find it much
easier to see the exact args in the docs, rather than seeing *args /
**kwargs and then navigating the inheritance path to find out all supported
arguments.
I'm not quite sure what you're referring to in this case; but generally, I
your arg list says *args / **kwargs. This is how I tried to document, for
example, optionsForClientTLS.
Instead of

def __init__(self, resource, requestFactory=None, *args, **kwargs):
http.HTTPFactory.__init__(self, *args, **kwargs)

you can have

def __init__(self, resource, logFile=None, requestFactory=None):
http.HTTPFactory.__init__(self, logFile=logFile)


Duplicating documentation is ugly... maybe we can "improve" pydoctor to
support something like this. Like @see but instead of creating a link, the
code is duplicated... but maybe a link is enough


def __init__(self, resource, logFile=None, requestFactory=None):
""""
Some description.

@include http.HTTPFactory.__init__.logFile
""""
http.HTTPFactory.__init__(self, logFile=logFile)
--
Adi Roiban
Tom Prince
2015-11-20 17:39:35 UTC
Permalink
Post by Glyph Lefkowitz
If it's been in a release, then there's probably nothing to do. It's
a shame that this went out, but once a breakage like this has happened
we have to live with it because otherwise, as you say, we'd be
breaking compatibility for the people that already upgraded. For
those that need to support both versions, keyword arguments are the
way to go.
There are perhaps a couple of things we can do. The types of the
arguments should usually (always?) be different, so we could at least
warn if we suspect the wrong thing was passed, if not either error out
or do the right thing, so code won't silently or inexcplicably fail
later. We could also deprecate passing an argument as a positional
argument, so eventually upgrading will always get an error, rather than
incorrect behavior.

Tom

Tom Prince
2015-11-20 06:54:49 UTC
Permalink
Post by Glyph Lefkowitz
Tom Prince discovered a regression on https://twistedmatrix.com/trac/ticket/7016#comment:14 <https://twistedmatrix.com/trac/ticket/7016#comment:14> - I think that this was introduced after 15.4, so it needs to be rolled back (or fixed, if someone can get to it before the revert) in 15.5.
Sadly, this has already been released (in 15.2), and changing it back
would also be an incompatible change. And, to set the record straight,
the issue was reported by sveinse on #twisted.

Tom
Continue reading on narkive:
Loading...