bpo-32940: IDLE: Simplify StringTranslatePseudoMapping in pyparse #5862

csabella · 2018-02-24T23:26:58Z

https://bugs.python.org/issue32940

…pyparse

the class as well as the instance.

terryjreedy

I think I exhausted my ideas for this issue. If CI passes, I expect to merge sometime tomorrow.

csabella · 2018-02-25T09:58:21Z

Lib/idlelib/pyparse.py

-    def get(self, key, default=None):
-        return self._get(key)
+    def __getitem__(self, key):
+        return self.get(key, 'x')


I had done a version yesterday that inherited from dict and only implemented:

class Parse(dict): def __missing__(self, key): return 'x'

It didn't have get or __getitem__ functionality on missing values, but it worked for the translate. But then I read on SO that's it's generally not good to inherit directly from dict and to use Mapping or MutableMapping instead. I guess this doesn't matter here, but I did notice that inheriting from dict allows some attributes to change from read-only to not, such as _tran.update = {1: 2}. So when is it OK to inherit directly from dict?

When it works. We are not going to do anything so nonsensical.

Defining __missing__ looks the most natural and efficient way. This is the case for which __missing__ was introduced.

csabella · 2018-02-25T09:58:54Z

Lib/idlelib/pyparse.py

-class StringTranslatePseudoMapping(Mapping):
-    r"""Utility class to be used with str.translate()
+class ParseMap(dict):
+    r"""Dict subclass that maps anything not in dict to 'x'.


Thought of this while I was sleeping. Should be class _ParseMap?

No. PEP434 declares nearly all of idlelib 'private', and it became such in practice nearly 3 years later, for in 3.6. There is no point to further privatization.

csabella · 2018-02-25T12:50:26Z

I added a commit to remove the Mapping import.

pppery · 2018-02-25T15:30:17Z

Lib/idlelib/pyparse.py


-class StringTranslatePseudoMapping(Mapping):
-    r"""Utility class to be used with str.translate()
+class ParseMap(dict):


Isn't this class equivalent to defaultdict(lambda:"x")

This class doesn't add the keys to the dictionary when they haven't been found. It will return the default value without growing the dictionary.

Also, defaultdict calls the function to get 'x', instead of just returning it.

serhiy-storchaka · 2018-02-25T18:36:10Z

Lib/idlelib/pyparse.py

-    def get(self, key, default=None):
-        return self._get(key)
+    def __getitem__(self, key):
+        return self.get(key, 'x')


Defining __missing__ looks the most natural and efficient way. This is the case for which __missing__ was introduced.

serhiy-storchaka · 2018-02-25T18:38:07Z

Lib/idlelib/pyparse.py

+    # Map1 maps uninteresting chars to 'x', open brackets to '(',
+    # close brackets to ')', and preserves quotes,  backslashes,
+    # newlines and hashes.  Used for str.translate() in _study1().
+    map1 = ParseMap()


What is the reason of renaming _tran to map1?

The tests pass with def __missing__(self, key): return 'x', so I will switch if at least as fast, which I suspect it is. (Working on timeit test)

map1 reads better to me. The object is a map for _study1. 'tran' is a prefix for numerous words, often verbs, not a word in itself.

This is a matter of style, but to me, _tran looks more semantic. It refers to the purpose of it (a translation table). map1 refers only to its type. This is like naming the integer variable "int1" instead of "cnt" or "col".

serhiy-storchaka · 2018-02-28T20:44:24Z

Lib/idlelib/pyparse.py

-        self._get = _get
+    # Calling this triples access time; see bpo-32940
+    def __missing__(self, key):
+        return 120  # ord('x')


Return just 'x'. It would be make sense to use b'x'[0] if integers have advantage, but I tested and didn't see a difference.

It is worth to remove ord() from values for other keys.

My tests, on the issue, show that ints slightly win. I see no need to change.

serhiy-storchaka · 2018-02-28T20:48:04Z

Lib/idlelib/pyparse.py

-    def get(self, key, default=None):
-        return self._get(key)
+# Map all ascii to 120 to avoid __missing__ call, then replace some.
+trans = ParseMap((i,120) for i in range(128))


PEP 8 requires a space after comma.

But wouldn't ParseMap.fromkeys(range(128), 'x') look clearer?

serhiy-storchaka · 2018-02-28T20:49:53Z

Lib/idlelib/pyparse.py

-    >>> mapping = StringTranslatePseudoMapping(preserve_dict, ord('x'))
-    >>> text = "a + b\tc\nd"
-    >>> text.translate(mapping)
+    >>> keepwhite = ParseMap((ord(c),ord(c)) for c in ' \t\n\r')


Add a space after comma. And the second ord() is not needed.

space added

miss-islington · 2018-02-28T22:24:00Z

Thanks @csabella for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

…thonGH-5862) The new code also runs faster. (cherry picked from commit f0daa88) Co-authored-by: Cheryl Sabella <[email protected]>

bedevere-bot · 2018-02-28T22:24:13Z

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

bedevere-bot · 2018-02-28T22:25:13Z

GH-5941 is a backport of this pull request to the 3.6 branch.

…thonGH-5862) The new code also runs faster. (cherry picked from commit f0daa88) Co-authored-by: Cheryl Sabella <[email protected]>

…-5862) The new code also runs faster. (cherry picked from commit f0daa88) Co-authored-by: Cheryl Sabella <[email protected]>

bpo-32940: IDLE: Simplify and rename StringTranslatePseudoMapping in …

0d46d85

…pyparse

csabella requested a review from terryjreedy as a code owner February 24, 2018 23:26

the-knights-who-say-ni added the CLA signed label Feb 24, 2018

bedevere-bot added the awaiting review label Feb 24, 2018

terryjreedy added 4 commits February 24, 2018 21:31

Simplify ParseMap to dict subclass

9472ee9

Simplify ParseMap test.

6e41844

Add comment

47cd451

Move test_map1 to ParseMapTest since half its purpose is to test

437ed57

the class as well as the instance.

terryjreedy added needs backport to 3.6 labels Feb 25, 2018

terryjreedy approved these changes Feb 25, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Feb 25, 2018

csabella commented Feb 25, 2018

View reviewed changes

Remove the import for Mapping

ef77d35

pppery reviewed Feb 25, 2018

View reviewed changes

terryjreedy added 2 commits February 25, 2018 13:31

Fix 'deecorator' typo in test/test_dataclasses

272f732

Fix 'deecorator' typo in test/test_dataclasses

909989c

serhiy-storchaka reviewed Feb 25, 2018

View reviewed changes

terryjreedy added 6 commits February 25, 2018 15:27

Merge remote-tracking branch 'origin/master'

7ad8392

Merge remote-tracking branch 'upstream/master'

968c4a8

Merge remote-tracking branch 'upstream/master'

d4e350a

Merge remote-tracking branch 'upstream/master'

74a9796

Merge remote-tracking branch 'origin/master' into pr_5862

405c5f7

Use __missing__ instead of __getitem__

867e897

serhiy-storchaka reviewed Feb 28, 2018

View reviewed changes

Add space, use fromkeys()

5c7ba8b

terryjreedy merged commit f0daa88 into python:master Feb 28, 2018

bedevere-bot removed the awaiting merge label Feb 28, 2018

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

bedevere-bot removed the needs backport to 3.6 label Feb 28, 2018

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

bpo-32940: IDLE: Simplify StringTranslatePseudoMapping in pyparse (GH…

7e57634

…-5862) The new code also runs faster. (cherry picked from commit f0daa88) Co-authored-by: Cheryl Sabella <[email protected]>

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

bpo-32940: IDLE: Simplify StringTranslatePseudoMapping in pyparse (GH…

32f5392

…-5862) The new code also runs faster. (cherry picked from commit f0daa88) Co-authored-by: Cheryl Sabella <[email protected]>

csabella deleted the pyparse branch March 1, 2018 00:17

csabella mentioned this pull request Jul 27, 2022

IDLE: pyparse - simplify StringTranslatePseudoMapping #77121

Closed

Uh oh!

bpo-32940: IDLE: Simplify StringTranslatePseudoMapping in pyparse #5862

bpo-32940: IDLE: Simplify StringTranslatePseudoMapping in pyparse #5862

Uh oh!

Conversation

csabella commented Feb 24, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryjreedy left a comment • 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!

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!

csabella commented Feb 25, 2018

Uh oh!

pppery Feb 25, 2018 • 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!

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 Feb 25, 2018 • 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!

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!

miss-islington commented Feb 28, 2018

Uh oh!

bedevere-bot commented Feb 28, 2018

Uh oh!

bedevere-bot commented Feb 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

csabella commented Feb 24, 2018 •

edited by bedevere-bot

Loading

terryjreedy left a comment •

edited

Loading

pppery Feb 25, 2018 •

edited

Loading

terryjreedy Feb 25, 2018 •

edited

Loading