Pierre Quentel <pierre.quen...@gmail.com> added the comment: @Victor
Thanks for the comments "- I don't understand why FieldStorage changes sys.stdout and sys.stderr (see remarks about IOMix above): please remove the charset argument (it is also confusing to have two encoding arguments). it should be done somewhere else" done "please remove the O_BINARY hack: the patch is for Python 3.2 and I closed issue #10841. If you would like a backport, another patch should be written later" done ""encoding = 'latin-1' # ?": write a real comment or remove it" I removed this part "'self.fp.read(...) # bytes': you should add a test on the type if you are not sure that fp.read() gives bytes" added tests in read_urlencoded(), read_multi() and read_binary() "file: the file(-like) object from which you can read the data *as bytes*": you should mention that TextIOWrapper is also tolerated (accepted?)" not done : here "file" is not the argument passed to the FieldStorage constructor, but the attribute of values returned from calls to FieldStorage. In the new implementation, its read() method always returns bytes "you may set fp directly to sys.stdin.buffer (instead of sys.stdin) if fp is None (it will be easier after removing the O_BINARY thing)" done "the patch adds a tab in an empty line, please don't do that :-)" done (hopefully :-) "you should add a (private?) attribute to FieldStorage to decide if it works on bytes or unicode, instead of using "self.filename is not None" test (eg. self._use_bytes = (self.filename is not None)" done "i don't like the idea of having a generic self.__write() method supporting bytes and unicode. it would prefer two methods, eg. self.__write_text() and self.__write_binary() (they can share a third private method)" not done, the argument of __write is always bytes "i don't like "stream_encoding" name: what is the "stream" here? do you process a "file", a "string" or a "stream"? why not just "self.encoding"?" done - "import email.parser,email.feedparser" one import is useless here. I prefer "from email.feedparser import FeedParser" because you get directly a ImportError if the symbol is missing. And it's already faster to get FeedParser instead of email.feedparser.FeedParser in a loop (dummy micro-optimization) done " even I like the following change, please do it in a separated patch: - if type(value) is type([]): + if isinstance(value,list):" not done "I really don't like the IOMix thing:" removed "I vote +0 to commit the patch now (if the release manager agrees), and +1 if all of my remarks are fixed." should be close to +0.8 now ;-) ---------- Added file: http://bugs.python.org/file20402/cgi_32.patch _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue4953> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
