bpo-30296: Partially revert #1489. by serhiy-storchaka · Pull Request #2624 · python/cpython

serhiy-storchaka · 2017-07-07T19:23:12Z

https://bugs.python.org/issue30296

mention-bot · 2017-07-07T19:23:17Z

@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zestyping, @pitrou and @loewis to be potential reviewers.

rhettinger · 2017-12-31T09:14:19Z

        if self._addresses is None:
-            self._addresses = tuple(address for group in self._groups
-                                            for address in group.addresses)
+            self._addresses = tuple([address for group in self._groups


The list comprehension is not necessary and doesn't improve the code. This should stay as-is.

The generator expression is slower. If you are fine with tiny performance regression introduced by the previous change I'll keep it.

rhettinger · 2017-12-31T09:14:32Z

    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)])


The list comprehension is not necessary and doesn't improve the code. This should stay as-is.

rhettinger · 2017-12-31T09:15:02Z

        props = config.pop('.', None)
        # Check for valid identifiers
-        kwargs = dict((k, config[k]) for k in config if valid_ident(k))
+        kwargs = {k: config[k] for k in config if valid_ident(k)}


This is an improvement. Go ahead.

rhettinger · 2017-12-31T09:15:21Z

            factory = klass
        props = config.pop('.', None)
-        kwargs = dict((k, config[k]) for k in config if valid_ident(k))
+        kwargs = {k: config[k] for k in config if valid_ident(k)}


This is an improvement. Go ahead.

rhettinger · 2017-12-31T09:18:59Z


-    drive_letters = set('abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ')
+    drive_letters = set('abcdefghijklmnopqrstuvwxyz'
+                        'ABCDEFGHIJKLMNOPQRSTUVWXYZ')


This change is unnecessary and not an improvement. Please leave as-is.
Adding a multi-line statement with implicit string concatenation doesn't make the code simpler.

This line is too long and don't conform PEP 8.

rhettinger · 2017-12-31T09:21:36Z

        else:
            raise TurtleGraphicsError("bad colorstring: %s" % cstr)
-        return tuple(c * self._colormode/255 for c in cl)
+        return tuple([c * self._colormode/255 for c in cl])


The list comprehension is not necessary and doesn't improve the code. This should stay as-is.

rhettinger · 2017-12-31T09:21:47Z

        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])


The list comprehension is not necessary and doesn't improve the code. This should stay as-is.

rhettinger · 2017-12-31T09:22:13Z


    with open("%s.py" % filename,"w") as f:
-        keys = sorted(x for x in docsdict.keys()
+        keys = sorted(x for x in docsdict


This is an improvement. Go ahead.

rhettinger · 2017-12-31T09:23:02Z


        headers = dict(req.unredirected_hdrs)
-        headers.update((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()


Please leave as-is. Don't create an unnecessary intermediate dictionary.

I afraid that this change changed semantic.

rhettinger · 2017-12-31T09:23:16Z

-        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')))])


The list comprehension is not necessary and doesn't improve the code. This should stay as-is.

bedevere-bot · 2017-12-31T09:23:34Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

serhiy-storchaka · 2017-12-31T09:51:12Z

Thank you for your review @rhettinger. Note that this PR mainly just reverts unnecessary changes that may have unwanted effects.

miss-islington · 2018-02-26T14:50:33Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-02-26T14:51:50Z

GH-5908 is a backport of this pull request to the 3.7 branch.

…ments. (pythonGH-2624) (cherry picked from commit 3f2e6f1) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…ments. (GH-2624) (cherry picked from commit 3f2e6f1) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bpo-30296: Partially revert python#1489.

d033141

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

ilevkivskyi approved these changes Jul 14, 2017

View reviewed changes

rhettinger requested changes Dec 31, 2017

View reviewed changes

bedevere-bot added the awaiting changes label Dec 31, 2017

serhiy-storchaka added 2 commits February 26, 2018 16:07

Merge branch 'master' into revert-pr-1489

ef627b1

Keep generator expressions for creating tuples.

becbbd6

serhiy-storchaka added the skip news label Feb 26, 2018

serhiy-storchaka merged commit 3f2e6f1 into python:master Feb 26, 2018

bedevere-bot removed the awaiting changes label Feb 26, 2018

serhiy-storchaka deleted the revert-pr-1489 branch February 26, 2018 14:50

serhiy-storchaka added the needs backport to 3.7 label Feb 26, 2018

bedevere-bot removed the needs backport to 3.7 label Feb 26, 2018

miss-islington added a commit that referenced this pull request Feb 26, 2018

Revert unneccessary changes made in bpo-30296 and apply other improve…

f8a3485

…ments. (GH-2624) (cherry picked from commit 3f2e6f1) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

Uh oh!

Uh oh!

Conversation

serhiy-storchaka commented Jul 7, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 31, 2017

Uh oh!

serhiy-storchaka commented Dec 31, 2017

Uh oh!

miss-islington commented Feb 26, 2018

Uh oh!

bedevere-bot commented Feb 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

serhiy-storchaka commented Jul 7, 2017 •

edited by bedevere-bot

Loading