Author rhettinger
Recipients jdufresne, rhettinger, serhiy.storchaka, steven.daprano
Date 2017-05-07.16:20:17
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1494174017.19.0.330788274662.issue30296@psf.upfronthosting.co.za>
In-reply-to
Content
Jon, we usually try to avoid sweeping changes across the library and instead preference to focus on one module at a time (Guido calls this "holistic refactoring").  There is a risk of introducing bugs and of destabiliizing code that has sat untouched but working for a long time.

Also, as Serhiy and Steven have pointed out, there are sometimes subtle reasons to prevent the existing code.  For example, I find the original to be clearer in this example:

 - return list(set(item.name for item in self.list))		 +        
 + return list({item.name for item in self.list})

and the original to be both clearer and faster in this example:

 - return list(map(_convert, node.elts))
 + return [_convert(v) for v in node.elts]

In some cases, using "dict(...)" or "set(...)" is clearer about the intension or more readable because it is parallel to the surrounding code (such as the parallelism of successive line in ast.py).

In some cases such as _pydecimal we have a aversion to changing code that aspires to be backwards compatible across many versions.

That said, there are a few places where it might be worthwhile for making a change.  Converting to set literals is almost always a win:

 -    interesting = set(("include", "lib", "libpath", "path"))		 
 +    interesting = {"include", "lib", "libpath", "path"}

As Serhiy noted, in some places using map() would be an improvement, so we should look at doing a few of those (where there is a win in term of speed, space, and readability):

 - version_info = tuple([int(x) for x in version.split(".")])
 + version_info = tuple(map(int, version.split(".")))

Overall, I think most of these shouldn't be done, but I will mark a few where some change might be considered an improvement.  Steven or Serhiy are both welcome to veto any one of those.  Likewise, if the original author of the code is still active, they have a veto as well.  The status quo usually wins until we're refactoring a single module and make improvements while thinking about that module.

You picked a difficult first patch because you picked a topic that we usually avoid (wholesale sweeps through the standard library) and an area where deciding "what is best" involves subtleties with respect to clarity, parallelism with surrounding code, speed, clear intention, back compatibility, and how the original author thinks about the code.
History
Date User Action Args
2017-05-07 16:20:17rhettingersetrecipients: + rhettinger, steven.daprano, serhiy.storchaka, jdufresne
2017-05-07 16:20:17rhettingersetmessageid: <1494174017.19.0.330788274662.issue30296@psf.upfronthosting.co.za>
2017-05-07 16:20:17rhettingerlinkissue30296 messages
2017-05-07 16:20:17rhettingercreate