[Python-Dev] a bunch of Patch reviews
Hello
I've looked at one bug and a bunch of patches and
added a comment to them:
(bug) [ 1102649 ] pickle files should be opened in binary mode
Added a comment about a possible different solution
[ 946207 ] Non-blocking Socket Server
Useless, what are the mixins for? Recommend close
[ 756021 ] Allow socket.inet_aton('255.255.255.255') on Windows
Looks good but added suggestion about when to test for special case
[ 740827 ] add urldecode() method to urllib
I think it's better to group these things into urlparse
[ 579435 ] Shadow Password Support Module
Would be nice to have, I recently just couldn't do the user
authentication that I wanted: based on the users' unix passwords
[ 1093468 ] socket leak in SocketServer
Trivial and looks harmless, but don't the sockets
get garbage collected once the request is done?
[ 1049151 ] adding bool support to xdrlib.py
Simple patch and 2.4 is out now, so...
It would be nice if somebody could have a look at my
own patches or help me a bit with them:
[ 1102879 ] Fix for 926423: socket timeouts + Ctrl-C don't play nice
[ 1103213 ] Adding the missing socket.recvall() method
[ 1103350 ] send/recv SEGMENT_SIZE should be used more in socketmodule
[ 1062014 ] fix for 764437 AF_UNIX socket special linux socket names
[ 1062060 ] fix for 1016880 urllib.urlretrieve silently truncates dwnld
Some of them come from the last Python Bug Day, see
http://www.python.org/moin/PythonBugDayStatus
Thank you !
Regards,
--Irmen de Jong
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
[Python-Dev] A bunch of patch reviews
Below are a set of five patch reviews. I don't have any patch to push for at
this point, so these patch reviews are just for you to read and enjoy. Thanks
everybody for developing and maintaining Python. I wouldn't know what to do
without it.
--Michiel.
Patch [ 853890 ] Optional keyword unicode args not handled correctly
The skipitem function in Python/getargs.c misses code for several argument
formats. This patch solves one of them ('u' and 'u#'), patch 985713 solves
another one ('w'). There are several more that need to be solved. I've asked the
patch contributor to write a complete patch, including all missing formats. This
patch is rather straightforward and solves a serious problem, so I'd recommend
accepting it once it is complete.
Patch [ 1167316 ] a fix for doctest crash when it is ran on itself
doctest.py in Lib fails when it is run on itself. The error is due to missing
"import doctest" statements and similar small problems in the doctest
docstrings. Patch seems to work correctly.
Patch [ 1166780 ] Fix _tryorder in webbrowser.py
In webbrowser.py, a user can override the default list of web browsers to be
opened by setting the BROWSER variable. Currently, if BROWSER is set, the
default list is not used at all. The patch contributor notes that if BROWSER is
set incorrectly, this may result in no browser being opened at all. While this
is true, seeing a different browser open instead of the one specified in BROWSER
may lead to confusion, and may lead to future bug reports saying "webbrowser.py
ignores the BROWSER variable" if a user sets BROWSER incorrectly. So my
suggestion is to at least print a warning message if the browser specified in
BROWSER cannot be used, and then proceed by opening one of the default browsers.
Patch [ 764221 ] add set/getattr interface to tkFont.Font objects
This patch has already made it into Python2.4, so I think this patch can be
closed.
Patch [ 1163314 ] the quotes page on the Web site links to something broken
The patch writer is correct that the link is broken, but it's not a Python bug.
I've suggested him to write to the web master.
--
Michiel de Hoon, Assistant Professor
University of Tokyo, Institute of Medical Science
Human Genome Center
4-6-1 Shirokane-dai, Minato-ku
Tokyo 108-8639
Japan
http://bonsai.ims.u-tokyo.ac.jp/~mdehoon
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
If someone could take a look at:
[ 1069624 ] incomplete support for AF_PACKET in socketmodule.c
I have to ship my own patched copy of the socket module because of
this... :|
On Sun, 2005-01-16 at 17:08 +0100, Irmen de Jong wrote:
> Hello
> I've looked at one bug and a bunch of patches and
> added a comment to them:
>
> (bug) [ 1102649 ] pickle files should be opened in binary mode
> Added a comment about a possible different solution
>
> [ 946207 ] Non-blocking Socket Server
> Useless, what are the mixins for? Recommend close
>
> [ 756021 ] Allow socket.inet_aton('255.255.255.255') on Windows
> Looks good but added suggestion about when to test for special case
>
> [ 740827 ] add urldecode() method to urllib
> I think it's better to group these things into urlparse
>
> [ 579435 ] Shadow Password Support Module
> Would be nice to have, I recently just couldn't do the user
> authentication that I wanted: based on the users' unix passwords
>
> [ 1093468 ] socket leak in SocketServer
> Trivial and looks harmless, but don't the sockets
> get garbage collected once the request is done?
>
> [ 1049151 ] adding bool support to xdrlib.py
> Simple patch and 2.4 is out now, so...
>
>
>
> It would be nice if somebody could have a look at my
> own patches or help me a bit with them:
>
> [ 1102879 ] Fix for 926423: socket timeouts + Ctrl-C don't play nice
> [ 1103213 ] Adding the missing socket.recvall() method
> [ 1103350 ] send/recv SEGMENT_SIZE should be used more in socketmodule
> [ 1062014 ] fix for 764437 AF_UNIX socket special linux socket names
> [ 1062060 ] fix for 1016880 urllib.urlretrieve silently truncates dwnld
>
> Some of them come from the last Python Bug Day, see
> http://www.python.org/moin/PythonBugDayStatus
>
>
> Thank you !
>
> Regards,
>
> --Irmen de Jong
> ___
> Python-Dev mailing list
> Python-Dev@python.org
> http://mail.python.org/mailman/listinfo/python-dev
> Unsubscribe:
> http://mail.python.org/mailman/options/python-dev/gjc%40inescporto.pt
--
Gustavo J. A. M. Carneiro
<[EMAIL PROTECTED]> <[EMAIL PROTECTED]>
The universe is always one step beyond logic.
smime.p7s
Description: S/MIME cryptographic signature
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Gustavo J. A. M. Carneiro wrote: If someone could take a look at: [ 1069624 ] incomplete support for AF_PACKET in socketmodule.c The rule applies: five reviews, with results posted to python-dev, and I will review your patch. Regards, Martin ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
On Mon, 2005-01-17 at 23:12 +0100, "Martin v. Löwis" wrote: > Gustavo J. A. M. Carneiro wrote: > > If someone could take a look at: > > > > [ 1069624 ] incomplete support for AF_PACKET in socketmodule.c > > > The rule applies: five reviews, with results posted to python-dev, > and I will review your patch. Oh... sorry, I didn't know about any rules. /me hides in shame. -- Gustavo J. A. M. Carneiro <[EMAIL PROTECTED]> <[EMAIL PROTECTED]> The universe is always one step beyond logic ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Gustavo J. A. M. Carneiro wrote: Oh... sorry, I didn't know about any rules. My apologies - I had announced this (personal) rule a few times, so I thought everybody on python-dev knew. If you really want to push a patch, you can do so by doing your own share of work, namely by reviewing other's patches. If you don't, someone will apply your patch when he finds the time to do so. So if you can wait, it might be best to wait a few months (this won't go into 2.4 patch releases, anyway). I think Brett Cannon now also follows this rule; it really falls short enough in practice because (almost) nobody really wants to push his patch bad enough to put some work into it to review other patches. Regards, Martin ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Irmen de Jong wrote: Hello I've looked at one bug and a bunch of patches and added a comment to them: [...] [ 579435 ] Shadow Password Support Module Would be nice to have, I recently just couldn't do the user authentication that I wanted: based on the users' unix passwords I'm almost done with completing this thing. (including doc and unittest). However: 1- I can't add new files to this tracker item. Should I open a new patch and refer to it? 2- As shadow passwords can only be retrieved when you are root, is a unit test module even useful? 3- Should the order of the chapters in the documentation be preserved? I'd rather add spwd below pwd, but this pushes the other unix modules "1 down"... --Irmen ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Irmen de Jong wrote: 1- I can't add new files to this tracker item. Should I open a new patch and refer to it? Depends on whether you want tracker admin access (i.e. become a SF python project member). If you do, you could attach patches to bug reports not written by you. 2- As shadow passwords can only be retrieved when you are root, is a unit test module even useful? Probably not. Alternatively, introduce a "root" resource, and make that test depend on the presence of the root resource. 3- Should the order of the chapters in the documentation be preserved? I'd rather add spwd below pwd, but this pushes the other unix modules "1 down"... You could make it a subsection (e.g. "spwd -- shadow passwords") Not sure whether this would be supported by the processing tools; if not, inserting the module in the middle might be acceptable. In any case, what is important is that the documentation is added - it can always be rearranged later. Regards, Martin ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Irmen de Jong wrote: > 3- Should the order of the chapters in the documentation >be preserved? I'd rather add spwd below pwd, but >this pushes the other unix modules "1 down"... On Tuesday 18 January 2005 17:17, Martin v. Löwis wrote: > You could make it a subsection (e.g. "spwd -- shadow passwords") > Not sure whether this would be supported by the processing > tools; if not, inserting the module in the middle might be > acceptable. I see no reason not to insert it right after pwd module docs. The order of the sections is not a backward compatibility concern. :-) -Fred -- Fred L. Drake, Jr. ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Martin, Irmen de Jong wrote: 1- I can't add new files to this tracker item. Should I open a new patch and refer to it? Depends on whether you want tracker admin access (i.e. become a SF python project member). If you do, you could attach patches to bug reports not written by you. That sounds very convenient, thanks. Does the status of 'python project member' come with certain expectations that must be complied with ? ;-) 2- As shadow passwords can only be retrieved when you are root, is a unit test module even useful? Probably not. Alternatively, introduce a "root" resource, and make that test depend on the presence of the root resource. I'm not sure what this "resource" is actually. I have seen them pass on my screen when executing the regression tests (resource "network" is not enabled, etc) but never paid much attention to them. Are they used to select optional parts of the test suite that can only be run in certain conditions? In any case, what is important is that the documentation is added - it can always be rearranged later. I've copied and adapted the "pwd" module chapter. I'll try to have a complete patch ready tomorrow night. Bye, -Irmen. ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
[Martin asks whether Irmen wants to be a tracker admin on SF] [Irmen de Jong] > That sounds very convenient, thanks. > Does the status of 'python project member' come with > certain expectations that must be complied with ? ;-) If you're using Python, you're already required to comply with all of Guido's demands, this would just make it more official. Kinda like the difference in sanctifying cohabitation with a marriage ceremony . OK, really, the minimum required of Python project members is that they pay some attention to Python-Dev. >>> 2- As shadow passwords can only be retrieved when >>>you are root, is a unit test module even useful? >> Probably not. Alternatively, introduce a "root" resource, >> and make that test depend on the presence of the root resource. > I'm not sure what this "resource" is actually. > I have seen them pass on my screen when executing the > regression tests (resource "network" is not enabled, etc) > but never paid much attention to them. > Are they used to select optional parts of the test suite > that can only be run in certain conditions? That's right, where "the condition" is precisely that you tell regrtest.py to enable a (one or more) named resource. There's no intelligence involved. "Resource names" are arbitrary, and can be passed to regrtest.py's -u argument. See regrtest's docstring for details. For example, to run the tests that require the network resource, pass "-u network". Then it will run network tests, and regardless of whether a network is actually available. Passing "-u all" makes it try to run all tests. ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Martin v. Löwis wrote: I think Brett Cannon now also follows this rule; it really falls short enough in practice because (almost) nobody really wants to push his patch bad enough to put some work into it to review other patches. Yes, I am trying to support the rule, but my schedule is nutty right now so my turn-around time is rather long at the moment. -Brett ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Irmen de Jong wrote: That sounds very convenient, thanks. Ok, welcome to the project! Please let me know whether it "works". Does the status of 'python project member' come with certain expectations that must be complied with ? ;-) There are a few conventions that are followed more or less stringently. You should be aware of the things in the developer FAQ, http://www.python.org/dev/devfaq.html Initially, "new" developers should follow a "write-after-approval" procedure, i.e. they should not commit anything until they got somebody's approval. Later, we commit things which we feel confident about, and post other things to SF. For CVS, I'm following a few more conventions which I think are not documented anywhere. - Always add a CVS commit message - Add an entry to Misc/NEWS, if there is a new feature, or if it is a bug fix for a maintenance branch (I personally don't list bug fixed in the HEAD revision, but others apparently do) - When committing configure.in, always remember to commit configure also (and pyconfig.h.in if it changed; remember to run autoheader) - Always run the test suite before committing - If you are committing a bug fix, consider to backport it to maintenance branches right away. If you don't backport it immediately, it likely won't appear in the next release. At the moment, backports to 2.4 are encouraged; backports to 2.3 are still possible for a few more days. If you chose not to backport for some reason, document that reason in the commit message. If you plan to backport, document that intention in the commit message (I usually say "Will backport to 2.x") - In the commit message, always refer to the SF tracker id. In the tracker item, always refer to CVS version numbers. I use the script attached to extract those numbers from the CVS commit message, to paste them into the SF tracker. I probably forgot to mention a few things; you'll notice few enough :-) HTH, Martin ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
[Martin v. Löwis] ... > - Add an entry to Misc/NEWS, if there is a new feature, > or if it is a bug fix for a maintenance branch > (I personally don't list bug fixed in the HEAD revision, > but others apparently do) You should. In part this is to comply with license requirements: we're a derivative work from CNRI and BeOpen's Python releases, and their licenses require that we include "a brief summary of the changes made to Python". That certainly includes changes made to repair bugs. It's also extremely useful in practice to have a list of repaired bugs in NEWS! That saved me hours just yesterday, when trying to account for a Zope3 test that fails under Python 2.4 but works under 2.3.4. 2.4 NEWS pointed out that tuple hashing changed to close bug 942952, which I can't imagine how I would have remembered otherwise. ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Tim Peters wrote: It's also extremely useful in practice to have a list of repaired bugs in NEWS! I'm not convinced about that - it makes the NEWS file almost unreadable, as the noise is now so high if every tiny change is listed; it is very hard to see what the important changes are. Regards, Martin ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
[Tim Peters] >> It's also extremely useful in practice to have a list of repaired >> bugs in NEWS! [Martin v. Löwis] > I'm not convinced about that - it makes the NEWS file almost > unreadable, as the noise is now so high if every tiny change is > listed; it is very hard to see what the important changes are. My experience disagrees, and I gave a specific example from just the last day. High-level coverage of the important bits is served (and served well) by Andrew's "What's New in Python" doc. (Although I'll note that when I did releases, I tried to sort section contents in NEWS, to put the more important items at the top.) In any case, you snipped the other part here: a brief summary of changes is required by the licenses, and they don't distinguish between changes due to features or bugs. If, for example, we didn't note that tuple hashing changed in NEWS, we would be required to note that in some other file. NEWS is the historical place for it, and works fine for this purpose (according to me ). ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Tim Peters wrote: My experience disagrees, and I gave a specific example from just the last day. High-level coverage of the important bits is served (and served well) by Andrew's "What's New in Python" doc. (Although I'll note that when I did releases, I tried to sort section contents in NEWS, to put the more important items at the top.) I long ago stopped reading the NEWS file, because it is just too much text. However, if it is desirable to list any change in the NEWS file, I'm willing to comply. Regards, Martin ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Irmen de Jong wrote:
I've looked at one bug and a bunch of patches and
added a comment to them:
Thanks! I have now processed the ones for which I found guidance.
As for the remaining ones:
[ 756021 ] Allow socket.inet_aton('255.255.255.255') on Windows
Looks good but added suggestion about when to test for special case
So what to do about this? Wait whether he revises the patch?
Accept anyway? Update the patch myself?
[ 1103350 ] send/recv SEGMENT_SIZE should be used more in socketmodule
So what do you propose to do? AFAICT, there is no definition of
SEGMENT_SIZE in a TCP implementation, and I think we should not try
to make up a value.
IMO, Python should expose sockets more or less "as-is". If the system
has a flaw, Python should expose it instead of working around it.
[ 1062014 ] fix for 764437 AF_UNIX socket special linux socket names
Can you please elaborate the problem? What is a "special linux socket
name"?
Regardless, the comment of the other reviewer is also valid: any patch
needs documentation and test cases.
Regards,
Martin
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] a bunch of Patch reviews
Martin v. Löwis wrote:
> Irmen de Jong wrote:
>
>> I've looked at one bug and a bunch of patches and
>> added a comment to them:
>
>
> Thanks! I have now processed the ones for which I found guidance.
Thank you
> As for the remaining ones:
>
>> [ 756021 ] Allow socket.inet_aton('255.255.255.255') on Windows
>> Looks good but added suggestion about when to test for special case
>
>
> So what to do about this? Wait whether he revises the patch?
> Accept anyway? Update the patch myself?
I think I will revise the patch myself.
I was just waiting for some more input, but
since nobody replied, I'll just go ahead with it.
>> [ 1103350 ] send/recv SEGMENT_SIZE should be used more in socketmodule
>
>
> So what do you propose to do? AFAICT, there is no definition of
> SEGMENT_SIZE in a TCP implementation, and I think we should not try
> to make up a value.
Well, for the VMS platform a value has been made up...
Why make an exception for that one and not for Win32?
> IMO, Python should expose sockets more or less "as-is". If the system
> has a flaw, Python should expose it instead of working around it.
Is this the default way of treating system flaws, or should
they be considered on a per-case basis? I can imagine that
some system flaws are just plain stupid and are easy to
hide (or circumvent) in the Python implementation.
The recv/send segment size issue can have nasty results
on Win32, see the referenced bug report 853507
"socket.recv() raises MemoryError exception" which I think
is related to this.
(yes, I know that Tim marked that bug as wontfix)
Note: I'm not too experienced with Win32 programming and so I
don't have a very good argumentation for the buffer size issue
on this platform. If there is somebody with better understanding
of the issues involved here, please advise.
(it's just empirical knowledge that I have that leads me to
believe that win32's tcp implementation suffers from similar
recv/send size problems as VMS does-- for which a special
case was made in the code)
>> [ 1062014 ] fix for 764437 AF_UNIX socket special linux socket names
>
>
> Can you please elaborate the problem? What is a "special linux socket
> name"?
See bug report 764437. AF_UNIX sockets on Linux can have so-called
'kernel' socket names that don't show up in the filesystem (regular
unix domain sockets do). The current socketmodule doesn't treat
this special kind of unix domain sockets well.
This fix was submitted during the last python bug day you can
find some more info here too: http://www.python.org/moin/PythonBugDayStatus
> Regardless, the comment of the other reviewer is also valid: any patch
> needs documentation and test cases.
Yes, Johannes is right. I should have made docs and test cases
but didn't get around to doing that yet.
When my home network is fixed (router failure) I'll try to spend
some time improving the patches.
Regards
Irmen de Jong
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
