[Twisted-Python] svn-reorg blocking
I really want to get svn-reorg finished up, because lots of people really want me to release Twisted sumo, and releasing sumo without svn-reorg is going to be very scary. Unfortunately, there's something blocking svn-reorg's merge: We haven't figured out a way to get the test suite to actually run with it. Trial, in trunk, looks for packages by traversing the filesystem. This doesn't work with packages made up of __path__s, which is how svn-reorg works. svn-reorg includes some changes to trial that replaces filesystem traversal with package importing, and special support for __path__. However, this doesn't work when a package is unimportable. Twisted has (at least) two packages which for various reasons are unimportable depending on your environment: twisted.internet.iocpreactor and twisted.internet.serialport. Both of them are unimportable because their __init__.py files have code in them, which is against the coding standard. I think glyph mentioned elsewhere in this thread that the coding standard allows it for current public interfaces, but it actually only allows it for the purposes of backwards compatibility while refactoring a module into a package. for iocpreactor, this is easy enough to fix: all it does in __init__.py is "from proactor import install", which is easily replaced with def install(*a, **kw): from proactor import install return install(*a, **kw) However, serialport is much more devious: 'from serialport import *'. This then causes an error on any system that doesn't have the third-party "serial" module installed (which is imported by serialport.py). The best suggestion I've heard for fixing this one is to make serialport a single module (again) that defines all the classes conditionally, but I do think that at least as far as code organisation is concerned, the current way the files are laid out is nice. Can we try to come to a conclusion about this stuff soon? -- Twisted | Christopher Armstrong: International Man of Twistery Radix | -- http://radix.twistedmatrix.com | Release Manager, Twisted Project \\\V/// | -- http://twistedmatrix.com |o O| | w----v----w-+
On Wed, 26 Oct 2005 12:01:53 +1100, Christopher Armstrong <radeex@gmail.com> wrote:
Here's a suggestion. Move all files in serialport/ up one level. Put _'s in front of all the 'implementation' module names. Replace the 'serialport' package with the existing 'serialport' module, which has pretty much *exactly* the same code in it; the external interface and import names don't change, and the only negative impact is 5 more files in internet/ (considering the size of directories in, say, Apache or Linux, I don't think that this is anything to cry over).
Can we try to come to a conclusion about this stuff soon?
Shall I make the previous suggestion a "conclusion"? ;-)
On 10/26/05, glyph@divmod.com <glyph@divmod.com> wrote:
I refer you to James' post. I'm fairly happy with your solution (although I still think that structurally it is less pleasant that having the implementation modules be in a package), but I'll let you and James debate it. On 10/26/05, James Y Knight wrote <foom@fuhm.net> wrote:
The way I see it is that you put a horrible hack in trial to work around a problem that wouldn't happen if the code actually followed Twisted's coding convention. -- Twisted | Christopher Armstrong: International Man of Twistery Radix | -- http://radix.twistedmatrix.com | Release Manager, Twisted Project \\\V/// | -- http://twistedmatrix.com |o O| | w----v----w-+
On Wed, 26 Oct 2005 13:36:35 +1100, Christopher Armstrong <radeex@gmail.com> wrote:
I don't like 'serialport' being a package, but that is a totally separate issue. If James' changes actually fix the problem, let's just go with that and be done with it; if we make serialport not be a package later, then the need for the extra attribute will go away.
On 10/26/05, James Y Knight wrote <foom@fuhm.net> wrote:
In James' defense, Twisted's coding standard actually says what serialport is doing is OK. The portion of the standard that says this is my work, and I think the coding standard should be changed, because this is an awful hack, but one thing that can't be said about it is that it isn't officially endorsed :). I tend to agree with your general sentiment but I don't think that this issue is important enough to hold up the re-org. James has implemented a solution. Let's just use it.
On 26/10/05, glyph@divmod.com <glyph@divmod.com> wrote:
I strongly and vigorously oppose merging this branch until there are unit tests for James' modifications to Trial. Instead I recommend moving serialport up a level (as you suggested earlier). That appears to be the simplest thing that could possibly work. cheers, jml
On Wed, 26 Oct 2005 15:10:04 +1100, Jonathan Lange <jml@mumak.net> wrote:
Far be it for me to argue with the Maintainer Of The Tests. It's official, fiat - I just moved serialport up a level in trunk, I will move it in whatever other branches are required. Any other blocking issues?
On 10/26/05, glyph@divmod.com <glyph@divmod.com> wrote:
Nope -- I'll try to get some tests into svn-reorg soon for trial's __path__ support, then we can figure out how the hell we're going to merge that beast. -- Twisted | Christopher Armstrong: International Man of Twistery Radix | -- http://radix.twistedmatrix.com | Release Manager, Twisted Project \\\V/// | -- http://twistedmatrix.com |o O| | w----v----w-+
On 10/26/05, glyph@divmod.com <glyph@divmod.com> wrote:
Any other blocking issues?
Well, not only does the new __path__ stuff not have tests (which I had just sat down to remedy), but it's also apparently breaking existing trial tests (clearly seen with "trial twisted.trial" in svn-reorg branch). Not only is it breaking them, it's doing it in a way that one failing test is causing others to fail - I assume because of the state-changing nature of the __path__ support change - that of importing the packages instead of doing stuff by filesystem. This __path__ thing continues to annoy me for various reasons, and I'm having second thoughts about how possible it is. Can other people please get on IRC this weekend and bash heads about it with me? Maybe we can find an alternative solution that isn't so magical and unsupported by, well, everything. Or we can just figure out how to get the __path__ stuff working for reals. -- Twisted | Christopher Armstrong: International Man of Twistery Radix | -- http://radix.twistedmatrix.com | Release Manager, Twisted Project \\\V/// | -- http://twistedmatrix.com |o O| | w----v----w-+
On Oct 26, 2005, at 12:10 AM, Jonathan Lange wrote:
I strongly and vigorously oppose merging this branch until there are unit tests for James' modifications to Trial.
Nobody has suggested that such a thing should be done; there is no need for strong and vigorous opposition to a position nobody has advocated.
This statement makes no sense in this context. Fiddling with serialport does not remove the need to make changes to trial that require new unit tests. As I said before, I don't even think it removes the need for the attribute to mark ignorable subpackages either, but, I'm not going to argue that point further. James
On 27/10/05, James Y Knight <foom@fuhm.net> wrote:
My mistake. I remembered that there weren't tests for __path__ support in the middle of the email -- and by then I had already used the word "vigorously", which I've been aching to use in a sentence for some time. However, I still hold that the attribute to mark ignorable subpackages is currently unnecessary. If it turns out that the attribute *is* necessary, I'm more than happy to see it added. Still, delight in vocabulary and strong disagreement aren't excuses for sloppy logic and a disregard for the facts -- so I apologise. If it helps, I'll own that my paranoia springs from my experiences maintaining Trial in 2002-2003, when all people added "useful" features with little consideration (leading to bloat, leading to The Rewrite). While the circumstances have changed (not least because your code is considered), I'm still going to hang on to that paranoia, if only to slow things down. cheers, jml
On Oct 25, 2005, at 9:01 PM, Christopher Armstrong wrote:
I did, and even implemented it. IMO it's the only sensible thing that can be done for this situation. I speak, of course, of using an attribute on the package to let trial know which subpackages do not contain any tests. It is simple, it works. What more do you want? Even if we do some horrible hack to serialport/iocpreactor such that that such an attribute is not required for twisted, it'll likely be required for someone else's code. Things that aren't better ways: - moving around serialport code for the benefit of trial and to the detriment of itself. - making the serialport module be not a module and instead be some lazy import thingamajig. I do not see that having a package that cannot be imported is in any way worse than having a module which cannot be imported, except for this issue with trial. Thus, a way to specifically tell trial not to care is really all that's necessary. I urge you to not block the svn- reorg on finding a mystical better way. Instead, block upon having test cases for the trial changes that I have already made. James
On Wed, 26 Oct 2005 12:01:53 +1100, Christopher Armstrong <radeex@gmail.com> wrote:
Here's a suggestion. Move all files in serialport/ up one level. Put _'s in front of all the 'implementation' module names. Replace the 'serialport' package with the existing 'serialport' module, which has pretty much *exactly* the same code in it; the external interface and import names don't change, and the only negative impact is 5 more files in internet/ (considering the size of directories in, say, Apache or Linux, I don't think that this is anything to cry over).
Can we try to come to a conclusion about this stuff soon?
Shall I make the previous suggestion a "conclusion"? ;-)
On 10/26/05, glyph@divmod.com <glyph@divmod.com> wrote:
I refer you to James' post. I'm fairly happy with your solution (although I still think that structurally it is less pleasant that having the implementation modules be in a package), but I'll let you and James debate it. On 10/26/05, James Y Knight wrote <foom@fuhm.net> wrote:
The way I see it is that you put a horrible hack in trial to work around a problem that wouldn't happen if the code actually followed Twisted's coding convention. -- Twisted | Christopher Armstrong: International Man of Twistery Radix | -- http://radix.twistedmatrix.com | Release Manager, Twisted Project \\\V/// | -- http://twistedmatrix.com |o O| | w----v----w-+
On Wed, 26 Oct 2005 13:36:35 +1100, Christopher Armstrong <radeex@gmail.com> wrote:
I don't like 'serialport' being a package, but that is a totally separate issue. If James' changes actually fix the problem, let's just go with that and be done with it; if we make serialport not be a package later, then the need for the extra attribute will go away.
On 10/26/05, James Y Knight wrote <foom@fuhm.net> wrote:
In James' defense, Twisted's coding standard actually says what serialport is doing is OK. The portion of the standard that says this is my work, and I think the coding standard should be changed, because this is an awful hack, but one thing that can't be said about it is that it isn't officially endorsed :). I tend to agree with your general sentiment but I don't think that this issue is important enough to hold up the re-org. James has implemented a solution. Let's just use it.
On 26/10/05, glyph@divmod.com <glyph@divmod.com> wrote:
I strongly and vigorously oppose merging this branch until there are unit tests for James' modifications to Trial. Instead I recommend moving serialport up a level (as you suggested earlier). That appears to be the simplest thing that could possibly work. cheers, jml
On Wed, 26 Oct 2005 15:10:04 +1100, Jonathan Lange <jml@mumak.net> wrote:
Far be it for me to argue with the Maintainer Of The Tests. It's official, fiat - I just moved serialport up a level in trunk, I will move it in whatever other branches are required. Any other blocking issues?
On 10/26/05, glyph@divmod.com <glyph@divmod.com> wrote:
Nope -- I'll try to get some tests into svn-reorg soon for trial's __path__ support, then we can figure out how the hell we're going to merge that beast. -- Twisted | Christopher Armstrong: International Man of Twistery Radix | -- http://radix.twistedmatrix.com | Release Manager, Twisted Project \\\V/// | -- http://twistedmatrix.com |o O| | w----v----w-+
On 10/26/05, glyph@divmod.com <glyph@divmod.com> wrote:
Any other blocking issues?
Well, not only does the new __path__ stuff not have tests (which I had just sat down to remedy), but it's also apparently breaking existing trial tests (clearly seen with "trial twisted.trial" in svn-reorg branch). Not only is it breaking them, it's doing it in a way that one failing test is causing others to fail - I assume because of the state-changing nature of the __path__ support change - that of importing the packages instead of doing stuff by filesystem. This __path__ thing continues to annoy me for various reasons, and I'm having second thoughts about how possible it is. Can other people please get on IRC this weekend and bash heads about it with me? Maybe we can find an alternative solution that isn't so magical and unsupported by, well, everything. Or we can just figure out how to get the __path__ stuff working for reals. -- Twisted | Christopher Armstrong: International Man of Twistery Radix | -- http://radix.twistedmatrix.com | Release Manager, Twisted Project \\\V/// | -- http://twistedmatrix.com |o O| | w----v----w-+
On Oct 26, 2005, at 12:10 AM, Jonathan Lange wrote:
I strongly and vigorously oppose merging this branch until there are unit tests for James' modifications to Trial.
Nobody has suggested that such a thing should be done; there is no need for strong and vigorous opposition to a position nobody has advocated.
This statement makes no sense in this context. Fiddling with serialport does not remove the need to make changes to trial that require new unit tests. As I said before, I don't even think it removes the need for the attribute to mark ignorable subpackages either, but, I'm not going to argue that point further. James
On 27/10/05, James Y Knight <foom@fuhm.net> wrote:
My mistake. I remembered that there weren't tests for __path__ support in the middle of the email -- and by then I had already used the word "vigorously", which I've been aching to use in a sentence for some time. However, I still hold that the attribute to mark ignorable subpackages is currently unnecessary. If it turns out that the attribute *is* necessary, I'm more than happy to see it added. Still, delight in vocabulary and strong disagreement aren't excuses for sloppy logic and a disregard for the facts -- so I apologise. If it helps, I'll own that my paranoia springs from my experiences maintaining Trial in 2002-2003, when all people added "useful" features with little consideration (leading to bloat, leading to The Rewrite). While the circumstances have changed (not least because your code is considered), I'm still going to hang on to that paranoia, if only to slow things down. cheers, jml
On Oct 25, 2005, at 9:01 PM, Christopher Armstrong wrote:
I did, and even implemented it. IMO it's the only sensible thing that can be done for this situation. I speak, of course, of using an attribute on the package to let trial know which subpackages do not contain any tests. It is simple, it works. What more do you want? Even if we do some horrible hack to serialport/iocpreactor such that that such an attribute is not required for twisted, it'll likely be required for someone else's code. Things that aren't better ways: - moving around serialport code for the benefit of trial and to the detriment of itself. - making the serialport module be not a module and instead be some lazy import thingamajig. I do not see that having a package that cannot be imported is in any way worse than having a module which cannot be imported, except for this issue with trial. Thus, a way to specifically tell trial not to care is really all that's necessary. I urge you to not block the svn- reorg on finding a mystical better way. Instead, block upon having test cases for the trial changes that I have already made. James
participants (4)
-
Christopher Armstrong -
glyph＠divmod.com -
James Y Knight -
Jonathan Lange