bpo-30296 Remove unnecessary tuples, lists, sets, and dicts #1489

jdufresne · 2017-05-07T12:57:07Z

Replaced list() with list comprehension
Replaced dict() with dict comprehension
Replaced set() with set comprehension
Replaced builtin func() with func() when supported (e.g. any(), all(), tuple(), min(), &
max())

the-knights-who-say-ni · 2017-05-07T12:57:09Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

rhettinger · 2017-05-07T16:34:51Z

Lib/_weakrefset.py

@@ -157,19 +157,19 @@ def issubset(self, other):
    __le__ = issubset

    def __lt__(self, other):
-        return self.data < set(ref(item) for item in other)
+        return self.data < {ref(item) for item in other}


Let's use: set(map(ref, other)) for all of these.

rhettinger · 2017-05-07T16:36:41Z

Lib/distutils/msvc9compiler.py

@@ -255,7 +255,7 @@ def query_vcvarsall(version, arch="x86"):
    """Launch vcvarsall.bat and read the settings from its environment
    """
    vcvarsall = find_vcvarsall(version)
-    interesting = set(("include", "lib", "libpath", "path"))
+    interesting = {"include", "lib", "libpath", "path"}


Yes to this one. Set literals are always a win.

rhettinger · 2017-05-07T16:37:27Z

Lib/inspect.py

@@ -368,7 +368,7 @@ def classify_class_attrs(cls):

    mro = getmro(cls)
    metamro = getmro(type(cls)) # for attributes stored in the metaclass
-    metamro = tuple([cls for cls in metamro if cls not in (type, object)])
+    metamro = tuple(cls for cls in metamro if cls not in (type, object))


Yes, this one is fine.

I would keep the original code.

rhettinger · 2017-05-07T16:40:40Z

Lib/ntpath.py

@@ -611,14 +611,14 @@ def commonpath(paths):
        split_paths = [p.split(sep) for d, p in drivesplits]

        try:
-            isabs, = set(p[:1] == sep for d, p in drivesplits)
+            isabs, = {p[:1] == sep for d, p in drivesplits}


This code smells bad (both the original and revision). Can this be replaced with any(...).

rhettinger · 2017-05-07T16:45:28Z

Lib/optparse.py

@@ -770,7 +770,7 @@ def convert_value(self, opt, value):
            if self.nargs == 1:
                return self.check_value(opt, value)
            else:
-                return tuple([self.check_value(opt, v) for v in value])
+                return tuple(self.check_value(opt, v) for v in value)


This looks like a reasonable change, but optparse is essentially defunct so we should leave it alone.

rhettinger · 2017-05-07T17:30:11Z

Tools/stringbench/stringbench.py

@@ -1323,8 +1323,8 @@ def quick_replace_multiple_match(STR):
 _format_dict = { "thing":"THING", "place":"PLACE", "location":"LOCATION", }
 _format_bytes = bytes_from_str(_format)
 _format_unicode = unicode_from_str(_format)
-_format_dict_bytes = dict((bytes_from_str(k), bytes_from_str(v)) for (k,v) in _format_dict.items())
-_format_dict_unicode = dict((unicode_from_str(k), unicode_from_str(v)) for (k,v) in _format_dict.items())
+_format_dict_bytes = {bytes_from_str(k): bytes_from_str(v) for k, v in _format_dict.items()}


I prefer not to touch a benchmark file lest it cause comparability issues.

rhettinger · 2017-05-07T17:31:06Z

setup.py

@@ -206,7 +206,7 @@ def build_extensions(self):
        extensions = [ext for ext in self.extensions
                      if ext.name not in disabled_module_list]
        # move ctypes to the end, it depends on other modules
-        ext_map = dict((ext.name, i) for i, ext in enumerate(extensions))
+        ext_map = {ext.name: i for i, ext in enumerate(extensions)}


Skip this. I prefer not to touch anything in setup.py.

rhettinger · 2017-05-07T17:31:23Z

setup.py

@@ -271,10 +271,10 @@ def build_extensions(self):
        for ext in self.extensions:
            self.check_extension_import(ext)

-        longest = max([len(e.name) for e in self.extensions], default=0)
+        longest = max((len(e.name) for e in self.extensions), default=0)


rhettinger · 2017-05-07T17:31:29Z

setup.py

        if self.failed or self.failed_on_import:
            all_failed = self.failed + self.failed_on_import
-            longest = max(longest, max([len(name) for name in all_failed]))
+            longest = max(longest, max(len(name) for name in all_failed))


rhettinger · 2017-05-07T17:31:48Z

setup.py

@@ -1162,8 +1162,7 @@ class db_found(Exception): pass
                    r'\s*.*#\s*.*define\s.*SQLITE_VERSION\W*"([\d\.]*)"', incf)
                if m:
                    sqlite_version = m.group(1)
-                    sqlite_version_tuple = tuple([int(x)
-                                        for x in sqlite_version.split(".")])
+                    sqlite_version_tuple = tuple(int(x) for x in sqlite_version.split("."))


Ambivalent.

terryjreedy

I keep idlelib 3.6 and 3.7 in sync as much as possible.
Please remove all Lib/idlelib/multicall.py changes from this PR and optionally (and preferably) make the 2 approved changes in a separate PR that can be backported easily.

terryjreedy · 2017-05-12T20:38:51Z

Lib/idlelib/multicall.py

-                         for name in _modifiers[number]])
+_modifier_names = {name: number
+                   for number in range(len(_modifiers))
+                   for name in _modifiers[number]}


I prefer the change as easier to read and likely faster. Ditto for the other dict(list) to dict comp change in this file. (Agreeing with Raymond.)

terryjreedy · 2017-05-12T20:39:48Z

Lib/idlelib/multicall.py

@@ -134,7 +134,7 @@ def nbits(n):
        return nb
    statelist = []
    for state in states:
-        substates = list(set(state & x for x in states))
+        substates = list({state & x for x in states})


Agree with Raymond, leave as is.

jdufresne · 2017-05-13T15:12:34Z

Thanks for the reviews! I believe I have addressed all feedback comments. If I missed or misunderstood any of the previous comments, please let me know.

* Replaced list(<generator expression>) with list comprehension * Replaced dict(<generator expression>) with dict comprehension * Replaced set(<list literal>) with set literal * Replaced builtin func(<list comprehension>) with func(<generator expression>) when supported (e.g. any(), all(), tuple(), min(), & max())

serhiy-storchaka · 2017-05-18T14:11:18Z

Lib/email/headerregistry.py

-            self._addresses = tuple([address for group in self._groups
-                                             for address in group.addresses])
+            self._addresses = tuple(address for group in self._groups
+                                            for address in group.addresses)
        return self._addresses



The original variant, with list comprehension, is faster than the variant with a generator. The difference is up to 2 times for simple tests.

I would keep the original code.

serhiy-storchaka · 2017-05-18T14:11:46Z

Lib/inspect.py

@@ -368,7 +368,7 @@ def classify_class_attrs(cls):

    mro = getmro(cls)
    metamro = getmro(type(cls)) # for attributes stored in the metaclass
-    metamro = tuple([cls for cls in metamro if cls not in (type, object)])
+    metamro = tuple(cls for cls in metamro if cls not in (type, object))


I would keep the original code.

serhiy-storchaka · 2017-05-18T14:15:08Z

Lib/logging/config.py

@@ -463,7 +463,7 @@ def configure_custom(self, config):
            c = self.resolve(c)
        props = config.pop('.', None)
        # Check for valid identifiers
-        kwargs = dict([(k, config[k]) for k in config if valid_ident(k)])
+        kwargs = dict((k, config[k]) for k in config if valid_ident(k))


I would use a dict comprehension.

And the same for other change in this file.

serhiy-storchaka · 2017-05-18T14:30:29Z

Lib/pstats.py

@@ -500,8 +500,7 @@ def add_callers(target, source):
        if func in new_callers:
            if isinstance(caller, tuple):
                # format used by cProfile
-                new_callers[func] = tuple([i[0] + i[1] for i in
-                                           zip(caller, new_callers[func])])
+                new_callers[func] = tuple(i[0] + i[1] for i in zip(caller, new_callers[func]))


I would keep the original code.

Maybe unpack a tuple:

tuple([i + j for i, j in zip(caller, new_callers[func])])

serhiy-storchaka · 2017-05-18T14:36:36Z

Lib/symtable.py

@@ -119,8 +119,8 @@ class Function(SymbolTable):
    __globals = None

    def __idents_matching(self, test_func):
-        return tuple([ident for ident in self.get_identifiers()
-                      if test_func(self._table.symbols[ident])])
+        return tuple(ident for ident in self.get_identifiers()


I would keep the original code.

serhiy-storchaka · 2017-05-18T14:39:24Z

Lib/turtle.py

@@ -2989,7 +2989,7 @@ def _getshapepoly(self, polygon, compound=False):
            t11, t12, t21, t22 = l, 0, 0, l
        elif self._resizemode == "noresize":
            return polygon
-        return tuple([(t11*x + t12*y, t21*x + t22*y) for (x, y) in polygon])
+        return tuple((t11*x + t12*y, t21*x + t22*y) for (x, y) in polygon)


I would keep the original code.

serhiy-storchaka · 2017-05-18T14:42:12Z

Lib/turtle.py

@@ -3839,8 +3839,8 @@ def write_docstringdict(filename="turtle_docstringdict"):
        docsdict[key] = eval(key).__doc__

    with open("%s.py" % filename,"w") as f:
-        keys = sorted([x for x in docsdict.keys()
-                            if x.split('.')[1] not in _alias_list])
+        keys = sorted(x for x in docsdict.keys()


.keys() can be removed. Iterate just docsdict.

serhiy-storchaka · 2017-05-18T14:48:36Z

Lib/urllib/request.py

@@ -845,7 +845,7 @@ def add_password(self, realm, uri, user, passwd):
            self.passwd[realm] = {}
        for default_port in True, False:
            reduced_uri = tuple(
-                [self.reduce_uri(u, default_port) for u in uri])
+                self.reduce_uri(u, default_port) for u in uri)


I would keep the original code.

serhiy-storchaka · 2017-05-18T14:52:09Z

Lib/urllib/request.py

@@ -1286,8 +1286,7 @@ def do_open(self, http_class, req, **http_conn_args):
        h.set_debuglevel(self._debuglevel)

        headers = dict(req.unredirected_hdrs)
-        headers.update(dict((k, v) for k, v in req.headers.items()
-                            if k not in headers))
+        headers.update((k, v) for k, v in req.headers.items() if k not in headers)


This changes the semantic. Updated headers is used in the generator expression. I don't know whether this is good.

I would use a dict comprehesion.

serhiy-storchaka · 2017-05-18T14:54:17Z

Tools/gdb/libpython.py

-        result = tuple([PyObjectPtr.from_pyobject_ptr(self[i]).proxyval(visited)
-                        for i in safe_range(int_from_int(self.field('ob_size')))])
+        result = tuple(PyObjectPtr.from_pyobject_ptr(self[i]).proxyval(visited)
+                       for i in safe_range(int_from_int(self.field('ob_size'))))


I would keep the original code.

the-knights-who-say-ni added the CLA not signed label May 7, 2017

jdufresne changed the title ~~Remove unnecessary tuples, lists, sets, and dicts~~ bpo-30296 Remove unnecessary tuples, lists, sets, and dicts May 7, 2017

rhettinger requested changes May 7, 2017

View reviewed changes

Mariatta removed the CLA not signed label May 8, 2017

the-knights-who-say-ni added the CLA signed label May 8, 2017

terryjreedy requested changes May 12, 2017

View reviewed changes

rhettinger approved these changes May 15, 2017

View reviewed changes

rhettinger merged commit 3972628 into python:master May 18, 2017

serhiy-storchaka reviewed May 18, 2017

View reviewed changes

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jul 7, 2017

bpo-30296: Partially revert python#1489.

d033141

jdufresne deleted the comprehensions branch November 18, 2017 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-30296 Remove unnecessary tuples, lists, sets, and dicts #1489

bpo-30296 Remove unnecessary tuples, lists, sets, and dicts #1489

jdufresne commented May 7, 2017

the-knights-who-say-ni commented May 7, 2017

rhettinger May 7, 2017

rhettinger May 7, 2017

rhettinger May 7, 2017

serhiy-storchaka May 18, 2017

rhettinger May 7, 2017

rhettinger May 7, 2017

rhettinger May 7, 2017

rhettinger May 7, 2017

rhettinger May 7, 2017

rhettinger May 7, 2017

rhettinger May 7, 2017

terryjreedy left a comment

terryjreedy May 12, 2017 •

edited

Loading

terryjreedy May 12, 2017 •

edited

Loading

jdufresne commented May 13, 2017

serhiy-storchaka May 18, 2017

serhiy-storchaka May 18, 2017

serhiy-storchaka May 18, 2017

serhiy-storchaka May 18, 2017

serhiy-storchaka May 18, 2017

serhiy-storchaka May 18, 2017

serhiy-storchaka May 18, 2017

serhiy-storchaka May 18, 2017

serhiy-storchaka May 18, 2017

serhiy-storchaka May 18, 2017

bpo-30296 Remove unnecessary tuples, lists, sets, and dicts #1489

bpo-30296 Remove unnecessary tuples, lists, sets, and dicts #1489

Conversation

jdufresne commented May 7, 2017

the-knights-who-say-ni commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy left a comment

Choose a reason for hiding this comment

terryjreedy May 12, 2017 • edited Loading

Choose a reason for hiding this comment

terryjreedy May 12, 2017 • edited Loading

Choose a reason for hiding this comment

jdufresne commented May 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy May 12, 2017 •

edited

Loading

terryjreedy May 12, 2017 •

edited

Loading