bpo-30296 Remove unnecessary tuples, lists, sets, and dicts by jdufresne · Pull Request #1489 · python/cpython

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

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

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

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

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

Yes, this one is fine.

I would keep the original code.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

I would keep the original code.

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

        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

                # 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

I would keep the original code.

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

I would keep the original code.

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

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

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

I would keep the original code.

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

        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

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

Uh oh!

Conversation

jdufresne commented May 7, 2017

Uh oh!

the-knights-who-say-ni commented May 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!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

terryjreedy May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdufresne commented May 13, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

terryjreedy May 12, 2017 •

edited

Loading

terryjreedy May 12, 2017 •

edited

Loading