[Twisted-Python] Some news about currently-pending code-reviews
First, a public service announcement about code-reviews: I'm not a Twisted Developer, I'm just a guy who submitted a patch and got talked into helping out by reviewing other people's Twisted patches while I was waiting for other people to review mine. You don't have to be a Twisted expert to help out, and don't worry about whether you're experienced enough to spot every possible problem - most patches get reviewed *at least* two or three times by different people, and every problem you pick up and the developer addresses is one less issue the next reviewer has to think about. As I've started reviewing over the past week or two, the developers whose code I've reviewed and the regulars in the #twisted IRC channel have all been super-helpful, and I highly recommend code-reviewing as a pleasant way to spend an idle afternoon. Everything you need to know to get started should be listed here: http://twistedmatrix.com/trac/wiki/ReviewProcess ---------------------------------------------------------------------- As I mentioned above, I've been doing Twisted code-reviews recently, and I've seen a few tickets in particular I thought the general Twisted community might like to know about, if not help out with. :) http://twistedmatrix.com/trac/ticket/3956 Add arraysize option to runQuery in adbapi - The "arraysize" property is the only writable cursor property defined by DBAPI2. Setting it can make the .fetchall() method faster with some DB modules, but Twisted's adbapi module offers no way to set it. - This ticket has a patch that adds an "cp_arraysize" keyword argument to ConnectionPool.runQuery() to set the .arraysize property, but this could interfere with DB modules that accept a "cp_arraysize" keyword parameter to cursor.execute(). - I looked up the API docs for as many Python DB modules as I could think of, and none of them attach any significance to a "cp_arraysize" keyword parameter. - However, at least cx_Oracle supports using keyword parameters for populating parameterised queries, like this: cursor.execute("select :name from dual;", name="Fred") So, my questions to the Twisted community are: - How many of you are using adbapi.ConnectionPool with cx_Oracle, or another DB module that supports arbitrary keyword arguments to cursor.execute? - Of those, how many of you are using parameterised queries or prepared statements with placeholders named "arraysize" or "cp_arraysize"? http://twistedmatrix.com/trac/ticket/4138 A fresh Twisted checkout should support "setup.py sdist" - Occasionally people ask about building RPMs of Twisted with "./setup.py bdist_rpm"; this ticket is the first step in getting that working - the rest of the process is #1696. - I'm not sure if it helps with other kinds of bdist, like "bdist_wininst" or "bdist_msi" - if it does, and you've been waiting for that, then you might want to help review the code and check it works properly on your platform! http://twistedmatrix.com/trac/ticket/4004 subunit reporter. FTW. - This ticket adds another test-reporter to Twisted Trial, one that outputs results in a format that can be parsed by the tools in the third-party "subunit" project, https://launchpad.net/subunit - Among other things, this would help integrate Python tests with test results from other systems (tap2subunit, subunit2junitxml), compare the results of different test-runs (subunit-diff), and enable cute GUI-based test-runners (subunit2gtk). - If subunit sounds like a tool you would find useful, you might want to help review the code!
http://twistedmatrix.com/trac/ticket/3956 Add arraysize option to runQuery in adbapi
- The "arraysize" property is the only writable cursor property defined by DBAPI2. Setting it can make the .fetchall() method faster with some DB modules, but Twisted's adbapi module offers no way to set it. - This ticket has a patch that adds an "cp_arraysize" keyword argument to ConnectionPool.runQuery() to set the .arraysize property, but
could interfere with DB modules that accept a "cp_arraysize" keyword parameter to cursor.execute(). - I looked up the API docs for as many Python DB modules as I could think of, and none of them attach any significance to a "cp_arraysize" keyword parameter. - However, at least cx_Oracle supports using keyword parameters for populating parameterised queries, like this:
cursor.execute("select :name from dual;", name="Fred")
So, my questions to the Twisted community are: - How many of you are using adbapi.ConnectionPool with cx_Oracle, or another DB module that supports arbitrary keyword arguments to cursor.execute? - Of those, how many of you are using parameterised queries or
[snip] this prepared
statements with placeholders named "arraysize" or "cp_arraysize"?
Well, as the guy who initiated this ticket, I'm certainly using adbapi.ConnectionPool with cx_Oracle. I'm not currently using any placeholders named "arraysize" or "cp_arraysize". This kind of informal poll *might* help us "rule out" using these (if someone says they're currently using them) - but it won't be very definitive; and this change will still technically be backwards-incompatible. Perhaps the only reasonable backwards-compatible change that could be made would be adding either an attribute or method to the ConnectionPool to set the arraysize. Gerrat
On Thu, Dec 24, 2009 at 09:41:11AM -0500, Gerrat Rickert wrote:
[snip]
http://twistedmatrix.com/trac/ticket/3956 Add arraysize option to runQuery in adbapi
Well, as the guy who initiated this ticket, I'm certainly using adbapi.ConnectionPool with cx_Oracle. I'm not currently using any placeholders named "arraysize" or "cp_arraysize".
But you are using the keyword-parameters-as-query-parameters extension that cx_Oracle provides?
This kind of informal poll *might* help us "rule out" using these (if someone says they're currently using them) - but it won't be very definitive; and this change will still technically be backwards-incompatible. Perhaps the only reasonable backwards-compatible change that could be made would be adding either an attribute or method to the ConnectionPool to set the arraysize.
I think the two positions here would be: a: adbapi.ConnectionPool is designed to wrap DBAPI2 modules; keyword parameters to cursor.execute() are not allowed in DBAPI2; therefore adbapi.ConnectionPool can use keyword parameters for itself. b: adbapi.ConnectionPool has never really enforced DBAPI2 compliance, so people have been using it with all kinds of crazy DBAPI2 extensions and we should allow people to keep doing so as much as possible. My cunning plan (which has somewhat backfired) was that one of these alternatives would seem sane, and one would seem ridiculous, and once the mailing list decided which was which I could go back to the ticket with that decision. The way things are at the moment, I'm leaning towards (b), but I believe the developer who's worked on the patch leans towards (a) and I don't feel I have the authority to demand a change of approach. I left the ticket awaiting review, in the hope that somebody with more authority or firmer opinions would come along to review it (it's a pretty small change!), but the ticket's been sitting there for weeks now - I felt I needed to do something more drastic to help it make progress.
On Thu, Dec 24, 2009 at 09:41:11AM -0500, Gerrat Rickert wrote:
[snip]
http://twistedmatrix.com/trac/ticket/3956 Add arraysize option to runQuery in adbapi
Well, as the guy who initiated this ticket, I'm certainly using adbapi.ConnectionPool with cx_Oracle. I'm not currently using any placeholders named "arraysize" or "cp_arraysize".
But you are using the keyword-parameters-as-query-parameters extension that cx_Oracle provides?
No, I am not. I probably didn't even notice this style was allowed, and likely wouldn't have used them even if I noticed. ('davep' mentioned on the ticket that he was using named binds, but didn't have an issue with using cp_arraysize as a keyword in runQuery) [snip]
I think the two positions here would be:
a: adbapi.ConnectionPool is designed to wrap DBAPI2 modules; keyword parameters to cursor.execute() are not allowed in DBAPI2; therefore adbapi.ConnectionPool can use keyword parameters for itself. b: adbapi.ConnectionPool has never really enforced DBAPI2 compliance, so people have been using it with all kinds of crazy DBAPI2 extensions and we should allow people to keep doing so as much as possible.
My cunning plan (which has somewhat backfired) was that one of these alternatives would seem sane, and one would seem ridiculous, and once the mailing list decided which was which I could go back to the ticket with that decision.
The way things are at the moment, I'm leaning towards (b), but I believe the developer who's worked on the patch leans towards (a) and I don't feel I have the authority to demand a change of approach. I left the ticket awaiting review, in the hope that somebody with more authority or firmer opinions would come along to review it (it's a pretty small change!), but the ticket's been sitting there for weeks now - I felt I needed to do something more drastic to help it make progress.
Thanks for trying to help push this along, Tim. I have no firm opinion either way. For me any solution is better than none. There doesn't seem to be any huge objections to using a "cp_arraysize" keyword param in runQuery, so it might not be the purest solution, but does seem practical.
On Dec 24, 2009, at 12:37 AM, Tim Allen wrote:
You don't have to be a Twisted expert to help out, and don't worry about whether you're experienced enough to spot every possible problem - most patches get reviewed *at least* two or three times by different people, and every problem you pick up and the developer addresses is one less issue the next reviewer has to think about.
Thanks for the encouragement, Tim. I've shied away from reviews as the first couple of tickets I tried to pick up, that were relevant to the things I *use* every day, were so far over my head (Twisted-wise) that I just kind of gave up. It was very strange to know and have used all of the concepts in the tickets, but have absolutely no idea what to *do* about it vis a vis the Twisted tickets in question. It was kind of like knowing all the nouns in a language but no verbs. I'll see if I can find some of the "easy" ones that are in some way related to my actual work and get started again... Thanks, Steve aka/ssteinerX aka/S
On Dec 24, 2009, at 12:37 AM, Tim Allen wrote:
As I've started reviewing over the past week or two, the developers whose code I've reviewed and the regulars in the #twisted IRC channel have all been super-helpful, and I highly recommend code-reviewing as a pleasant way to spend an idle afternoon. Everything you need to know to get started should be listed here:
Thanks for writing about this! It's always nice to hear about someone enjoying the review process :-).
participants (4)
-
Gerrat Rickert -
Glyph Lefkowitz -
ssteinerX＠gmail.com -
Tim Allen