Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(530)

Issue 107042: show python mimetypes module love, take 2

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 11 months ago by Jacob Rus
Modified:
8 years, 5 months ago
Base URL:
http://svn.python.org/view/*checkout*/python/trunk/
Visibility:
Public.

Description

Well, no one commented on my original uploaded patch, but here's a new one: Again, this is from: http://bugs.python.org/issue6626 Previous patch: http://codereview.appspot.com/104091/show

Patch Set 1 #

Total comments: 48
Unified diffs Side-by-side diffs Delta from patch set Stats (+371 lines, -437 lines) Patch
Lib/mimetypes.py View 3 chunks +369 lines, -435 lines 37 comments Download
Lib/test/test_mimetypes.py View 1 chunk +2 lines, -2 lines 11 comments Download

Messages

Total messages: 25
Jacob Rus
9 years, 11 months ago (2009-08-15 02:24:01 UTC) #1
Jacob Rus
copy some comments over from http://codereview.appspot.com/104091/diff/1/2 http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (left): http://codereview.appspot.com/107042/diff/1/2#oldcode222 Line 222: The way ...
9 years, 11 months ago (2009-08-15 03:46:59 UTC) #2
Antoine Pitrou
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode68 Line 68: def __init__(self, filenames=knownfiles, strict=True): Does `filenames=knownfiles` mean Apache ...
9 years, 11 months ago (2009-08-15 10:50:57 UTC) #3
Jacob Rus
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode68 Line 68: def __init__(self, filenames=knownfiles, strict=True): Yes, but it's not ...
9 years, 11 months ago (2009-08-15 12:02:36 UTC) #4
Jacob Rus
http://codereview.appspot.com/107042/diff/1/3 File Lib/test/test_mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/3#newcode15 Line 15: self.db = mimetypes.MimeTypes(filenames=[]) I very much doubt it. ...
9 years, 11 months ago (2009-08-15 12:05:23 UTC) #5
Jacob Rus
http://codereview.appspot.com/107042/diff/1/3 File Lib/test/test_mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/3#newcode15 Line 15: self.db = mimetypes.MimeTypes(filenames=[]) So to clarify, adding filenames=[] ...
9 years, 11 months ago (2009-08-15 12:07:21 UTC) #6
Antoine Pitrou
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode102 Line 102: Deprecated: Call MimeTypes.register([(ext, type)]) instead. But there's no ...
9 years, 11 months ago (2009-08-15 12:23:21 UTC) #7
Jacob Rus
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode102 Line 102: Deprecated: Call MimeTypes.register([(ext, type)]) instead. There is one ...
9 years, 11 months ago (2009-08-15 12:27:47 UTC) #8
Antoine Pitrou
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode102 Line 102: Deprecated: Call MimeTypes.register([(ext, type)]) instead. On 2009/08/15 12:27:47, ...
9 years, 11 months ago (2009-08-15 12:34:33 UTC) #9
Jacob Rus
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode102 Line 102: Deprecated: Call MimeTypes.register([(ext, type)]) instead. No, if you ...
9 years, 11 months ago (2009-08-15 12:45:22 UTC) #10
Jacob Rus
http://codereview.appspot.com/107042/diff/1/3 File Lib/test/test_mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/3#newcode15 Line 15: self.db = mimetypes.MimeTypes(filenames=[]) We aren't currently testing the ...
9 years, 11 months ago (2009-08-15 12:49:08 UTC) #11
Antoine Pitrou
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode102 Line 102: Deprecated: Call MimeTypes.register([(ext, type)]) instead. On 2009/08/15 12:45:22, ...
9 years, 11 months ago (2009-08-15 13:13:44 UTC) #12
Jacob Rus
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode102 Line 102: Deprecated: Call MimeTypes.register([(ext, type)]) instead. > Ok, then ...
9 years, 11 months ago (2009-08-15 13:29:25 UTC) #13
Jacob Rus
http://codereview.appspot.com/107042/diff/1/3 File Lib/test/test_mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/3#newcode15 Line 15: self.db = mimetypes.MimeTypes(filenames=[]) > When changing a module's ...
9 years, 11 months ago (2009-08-15 13:32:09 UTC) #14
Jacob Rus
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode406 Line 406: global register, encodings_map, suffix_map, types_map, common_types Whoops, that's ...
9 years, 11 months ago (2009-08-15 13:33:23 UTC) #15
Antoine Pitrou
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode102 Line 102: Deprecated: Call MimeTypes.register([(ext, type)]) instead. On 2009/08/15 13:29:25, ...
9 years, 11 months ago (2009-08-15 13:53:57 UTC) #16
Jacob Rus
http://codereview.appspot.com/107042/diff/1/2 File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/2#newcode102 Line 102: Deprecated: Call MimeTypes.register([(ext, type)]) instead. > By the ...
9 years, 11 months ago (2009-08-15 15:07:22 UTC) #17
Jacob Rus
http://codereview.appspot.com/107042/diff/1/3 File Lib/test/test_mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/3#newcode15 Line 15: self.db = mimetypes.MimeTypes(filenames=[]) No, the previous code tested ...
9 years, 11 months ago (2009-08-15 15:22:59 UTC) #18
Antoine Pitrou
On 2009/08/15 15:07:22, Jacob Rus wrote: > > Currently, most uses of this module call ...
9 years, 11 months ago (2009-08-15 16:11:32 UTC) #19
Nick Coghlan
http://codereview.appspot.com/107042/diff/1/3 File Lib/test/test_mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/3#newcode15 Line 15: self.db = mimetypes.MimeTypes(filenames=[]) I agree with Antoine here ...
9 years, 9 months ago (2009-10-10 13:42:35 UTC) #20
Nick Coghlan
As I said in one of my comments, I appreciate the desire for a cleaner ...
9 years, 9 months ago (2009-10-10 14:30:33 UTC) #21
Jacob Rus
Really though, the bigger question is about trying to actually bring the list of types ...
9 years, 9 months ago (2009-10-11 00:27:51 UTC) #22
merwok
http://codereview.appspot.com/107042/diff/1/Lib/mimetypes.py File Lib/mimetypes.py (right): http://codereview.appspot.com/107042/diff/1/Lib/mimetypes.py#newcode68 Lib/mimetypes.py:68: def __init__(self, filenames=knownfiles, strict=True): As Nick said, we have ...
8 years, 5 months ago (2011-02-09 23:13:53 UTC) #23
Jacob Rus
> I disagree. Antoine asked a legitimate question (for example, Google > Code Search sometimes ...
8 years, 5 months ago (2011-02-10 02:03:48 UTC) #24
merwok
8 years, 5 months ago (2011-02-10 23:07:38 UTC) #25
> We’re talking about (poorly engineered and buggy) implementation
> details, not documented interfaces.
Ah, okay.  That was not always clear in your patch and comments.

> In any case, the main things I wish we could discuss sometime were
> 
> (a) which changes don’t count as “breaking things”?
Changes that don’t make the test suite fail.

> As one small
> example, can we order the reading of possible Apache locations so that
> the locations for newer versions aren’t clobbered by out-of-date
> mimetypes databases from older versions?
I think that is a change good to have in a new version (for example
3.3), but it would probably be rejected in a bugfix release (like 3.1.4).

> (b) is there any reasonable way forward to make a better (i.e. not
> broken) API in this module [...]
If you have a clone of py3k around, it may be interesting to look at the
evolution of the configparser module in the last months.  It has gained
new features and documentation, some brittle implementation details have
been made more robust, but the external, documented API has been kept
compatible.  So yes, it is possible to add functions and classes,
deprecate existing code, revamp the implementation, but while preserving
compatibility.

> (c) is there any interest in including a somewhat real-world up-to-date
> set of mime type definitions?
Certainly.  For example, an entry for SVG has been added in 3.2.

> To be honest, I came away from the discussion a bit disheartened,
> because I was trying to say “look how we can improve this, bring it up
> to date without changing the documented API, make it flexible enough so
> that sizable projects don’t each have to reinvent it with their own new
> APIs, etc.” and the response seemed to be roughly “nah, we can’t do
> that, this module and all its problems are stuck for all time”.
I understand how you feel.  The response was not “we can’t do anything”,
rather “we can’t do this and that change you’re proposing, but we will
accept your edits if you rewrite them to not break existing uses”.  I do
hope you can see how we’re bound by the compatibility promise, and that
you will agree to adapt your patch.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b