Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
terryjreedy merged 15 commits into from
Feb 28, 2018
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0d46d85
bpo-32940: IDLE: Simplify and rename StringTranslatePseudoMapping in …
csabella Feb 24, 2018
9472ee9
Simplify ParseMap to dict subclass
terryjreedy Feb 25, 2018
6e41844
Simplify ParseMap test.
terryjreedy Feb 25, 2018
47cd451
Add comment
terryjreedy Feb 25, 2018
437ed57
Move test_map1 to ParseMapTest since half its purpose is to test
terryjreedy Feb 25, 2018
ef77d35
Remove the import for Mapping
csabella Feb 25, 2018
272f732
Fix 'deecorator' typo in test/test_dataclasses
terryjreedy Feb 25, 2018
909989c
Fix 'deecorator' typo in test/test_dataclasses
terryjreedy Feb 25, 2018
7ad8392
Merge remote-tracking branch 'origin/master'
terryjreedy Feb 25, 2018
968c4a8
Merge remote-tracking branch 'upstream/master'
terryjreedy Feb 26, 2018
d4e350a
Merge remote-tracking branch 'upstream/master'
terryjreedy Feb 27, 2018
74a9796
Merge remote-tracking branch 'upstream/master'
terryjreedy Feb 28, 2018
405c5f7
Merge remote-tracking branch 'origin/master' into pr_5862
terryjreedy Feb 28, 2018
867e897
Use __missing__ instead of __getitem__
terryjreedy Feb 28, 2018
5c7ba8b
Add space, use fromkeys()
terryjreedy Feb 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 13 additions & 42 deletions Lib/idlelib/idle_test/test_pyparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,44 +8,19 @@
from idlelib import pyparse


class StringTranslatePseudoMappingTest(unittest.TestCase):

@classmethod
def setUpClass(cls):
whitespace_chars = ' \t\n\r'
cls.preserve_dict = {ord(c): ord(c) for c in whitespace_chars}
cls.default = ord('x')
cls.mapping = pyparse.StringTranslatePseudoMapping(
cls.preserve_dict, default_value=ord('x'))

@classmethod
def tearDownClass(cls):
del cls.preserve_dict, cls.default, cls.mapping

def test__init__(self):
m = self.mapping
self.assertEqual(m._non_defaults, self.preserve_dict)
self.assertEqual(m._default_value, self.default)

def test__get_item__(self):
self.assertEqual(self.mapping[ord('\t')], ord('\t'))
self.assertEqual(self.mapping[ord('a')], self.default)

def test__len__(self):
self.assertEqual(len(self.mapping), len(self.preserve_dict))

def test__iter__(self):
count = 0
for key, value in self.mapping.items():
self.assertIn(key, self.preserve_dict)
count += 1
self.assertEqual(count, len(self.mapping))

def test_get(self):
self.assertEqual(self.mapping.get(ord('\t')), ord('\t'))
self.assertEqual(self.mapping.get('a'), self.default)
# Default is a parameter, but it isn't used.
self.assertEqual(self.mapping.get('a', default=500), self.default)
class ParseMapTest(unittest.TestCase):

def test_getitem(self):
keepwhite = {ord(c): ord(c) for c in ' \t\n\r'}
mapping = pyparse.ParseMap(keepwhite)
self.assertEqual(mapping[ord('\t')], ord('\t'))
self.assertEqual(mapping[ord('a')], 'x')

def test_map1(self):
# map1 is the production instance of ParseMap, used in _study1
parser = pyparse.Parser(4, 4)
self.assertEqual('\t a([{b}])b"c\'d\n'.translate(parser.map1),
'xxx(((x)))x"x\'x\n')


class PyParseTest(unittest.TestCase):
Expand Down Expand Up @@ -152,10 +127,6 @@ def test_set_lo(self):
p.set_lo(44)
self.assertEqual(p.code, code[44:])

def test_tran(self):
self.assertEqual('\t a([{b}])b"c\'d\n'.translate(self.parser._tran),
'xxx(((x)))x"x\'x\n')

def test_study1(self):
eq = self.assertEqual
p = self.parser
Expand Down
65 changes: 18 additions & 47 deletions Lib/idlelib/pyparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""Define partial Python code Parser used by editor and hyperparser.

Instances of StringTranslatePseudoMapping are used with str.translate.
Instances of ParseMap are used with str.translate.

The following bound search and match functions are defined:
_synchre - start of popular statement;
Expand Down Expand Up @@ -101,46 +101,19 @@
""", re.VERBOSE).match


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

@pppery pppery Feb 25, 2018 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@csabella csabella Feb 25, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@terryjreedy terryjreedy Feb 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

r"""Dict subclass that maps anything not in dict to 'x'.

@csabella csabella Feb 25, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@terryjreedy terryjreedy Feb 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.


This Mapping class wraps a given dict. When a value for a key is
requested via __getitem__() or get(), the key is looked up in the
given dict. If found there, the value from the dict is returned.
Otherwise, the default value given upon initialization is returned.
This is designed to be used with str.translate in study1.
Anything not specifically mapped otherwise becomes 'x'.
Example: replace everything except whitespace with 'x'.

This allows using str.translate() to make some replacements, and to
replace all characters for which no replacement was specified with
a given character instead of leaving them as-is.

For example, to replace everything except whitespace with 'x':

>>> whitespace_chars = ' \t\n\r'
>>> preserve_dict = {ord(c): ord(c) for c in whitespace_chars}
>>> 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')

@serhiy-storchaka serhiy-storchaka Feb 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@terryjreedy terryjreedy Feb 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space added

>>> "a + b\tc\nd".translate(keepwhite)
'x x x\tx\nx'
"""
def __init__(self, non_defaults, default_value):
self._non_defaults = non_defaults
self._default_value = default_value

def _get(key, _get=non_defaults.get, _default=default_value):
return _get(key, _default)
self._get = _get

def __getitem__(self, item):
return self._get(item)

def __len__(self):
return len(self._non_defaults)

def __iter__(self):
return iter(self._non_defaults)

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

@csabella csabella Feb 25, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@terryjreedy terryjreedy Feb 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@csabella csabella Feb 25, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-)

@serhiy-storchaka serhiy-storchaka Feb 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



class Parser:
Expand Down Expand Up @@ -224,15 +197,13 @@ def set_lo(self, lo):
if lo > 0:
self.code = self.code[lo:]

# Build a translation table to map uninteresting chars to 'x', open
# brackets to '(', close brackets to ')' while preserving quotes,
# backslashes, newlines and hashes. This is to be passed to
# str.translate() in _study1().
_tran = {}
_tran.update((ord(c), ord('(')) for c in "({[")
_tran.update((ord(c), ord(')')) for c in ")}]")
_tran.update((ord(c), ord(c)) for c in "\"'\\\n#")
_tran = StringTranslatePseudoMapping(_tran, default_value=ord('x'))
# 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()

@serhiy-storchaka serhiy-storchaka Feb 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason of renaming _tran to map1?

@terryjreedy terryjreedy Feb 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@serhiy-storchaka serhiy-storchaka Feb 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

map1.update((ord(c), ord('(')) for c in "({[")
map1.update((ord(c), ord(')')) for c in ")}]")
map1.update((ord(c), ord(c)) for c in "\"'\\\n#")

def _study1(self):
"""Find the line numbers of non-continuation lines.
Expand All @@ -250,7 +221,7 @@ def _study1(self):
# uninteresting characters. This can cut the number of chars
# by a factor of 10-40, and so greatly speed the following loop.
code = self.code
code = code.translate(self._tran)
code = code.translate(self.map1)
code = code.replace('xxxxxxxx', 'x')
code = code.replace('xxxx', 'x')
code = code.replace('xx', 'x')
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/IDLE/2018-02-24-18-20-50.bpo-32940.ZaJ1Rf.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Simplify and rename StringTranslatePseudoMapping in pyparse.