2to3 BottomMatcher patch

[gboutsioukis, 2010-08-20 12:17:09]

http://codereview.appspot.com/2013042/diff/1/2
File lib2to3/pytree.py (right):

http://codereview.appspot.com/2013042/diff/1/2#newcode221
lib2to3/pytree.py:221: 
The next two methods are actually not used in the final version and it's safe to
remove them.

http://codereview.appspot.com/2013042/diff/1/2#newcode311
lib2to3/pytree.py:311: for node in child.post_order():
That's a typo left from some early testing; it should be pre_order obviously.

[Benjamin, 2010-08-20 19:41:12]

It looks good. There are just some idomatic code issues.

http://codereview.appspot.com/2013042/diff/1/6
File lib2to3/btm_matcher.py (right):

http://codereview.appspot.com/2013042/diff/1/6#newcode11
lib2to3/btm_matcher.py:11: from .btm_utils import *
Avoid "*" imports, use qualified or explicitly listed names instead.

http://codereview.appspot.com/2013042/diff/1/6#newcode23
lib2to3/btm_matcher.py:23: def new_id(cls):
Consider just using itertools.count().

http://codereview.appspot.com/2013042/diff/1/6#newcode57
lib2to3/btm_matcher.py:57: if type(pattern[0]) is tuple:
isinstance(pattern[0], tuple)

http://codereview.appspot.com/2013042/diff/1/6#newcode69
lib2to3/btm_matcher.py:69: #single token
This section is indented weirdly.

http://codereview.appspot.com/2013042/diff/1/6#newcode71
lib2to3/btm_matcher.py:71: if pattern[0] not in start.transition_table.keys():
The keys() is redundant and be removed.

http://codereview.appspot.com/2013042/diff/1/6#newcode102
lib2to3/btm_matcher.py:102: results = {}
I suggest you make this a collections.defaultdict(list).

http://codereview.appspot.com/2013042/diff/1/6#newcode105
lib2to3/btm_matcher.py:105: while(current_ast_node):
Remove parenthesis.

http://codereview.appspot.com/2013042/diff/1/6#newcode109
lib2to3/btm_matcher.py:109: if hasattr(child, "value") and child.value==';':
This could be condensed to getattr(child, "value", u"") or even
isinstance(child, pytree.Leaf) and child.value == u";".

http://codereview.appspot.com/2013042/diff/1/6#newcode112
lib2to3/btm_matcher.py:112: if current_ast_node.type == 1:
Can you use token.NAME instead of hardcoding 1?

http://codereview.appspot.com/2013042/diff/1/6#newcode118
lib2to3/btm_matcher.py:118: if node_token in
current_ac_node.transition_table.keys():
keys() is redundant.

http://codereview.appspot.com/2013042/diff/1/6#newcode122
lib2to3/btm_matcher.py:122: if not fixer in results.keys():
ditto

http://codereview.appspot.com/2013042/diff/1/6#newcode129
lib2to3/btm_matcher.py:129: if current_ast_node.parent is not None \
Wrap with parenthesis instead of \.

http://codereview.appspot.com/2013042/diff/1/6#newcode135
lib2to3/btm_matcher.py:135: if node_token in
current_ac_node.transition_table.keys():
Redundant keys().

http://codereview.appspot.com/2013042/diff/1/4
File lib2to3/btm_utils.py (right):

http://codereview.appspot.com/2013042/diff/1/4#newcode7
lib2to3/btm_utils.py:7: syms = pattern_symbols.__dict__
Why can't you just use the attribute access of pattern_symbols?

http://codereview.appspot.com/2013042/diff/1/4#newcode21
lib2to3/btm_utils.py:21: def __init__(self, type=None, name=None, times=1):
Is the times argument ever used?

http://codereview.appspot.com/2013042/diff/1/4#newcode29
lib2to3/btm_utils.py:29: self.alternatives_number = None
Is this attribute ever used?

http://codereview.appspot.com/2013042/diff/1/4#newcode119
lib2to3/btm_utils.py:119: new_node = reduce_tree(node.children[0])
You could say node = node.children[0] and remove the elif in the next line to
avoid pointless recursion.

http://codereview.appspot.com/2013042/diff/1/4#newcode123
lib2to3/btm_utils.py:123: if len(node.children)<=2:
There should be space between compare operators and their operands.

http://codereview.appspot.com/2013042/diff/1/4#newcode126
lib2to3/btm_utils.py:126: elif len(node.children)>2:
Is this condition needed or could it just be an "else".

http://codereview.appspot.com/2013042/diff/1/4#newcode154
lib2to3/btm_utils.py:154: if hasattr(node.children[0], "value") and \
I think isinstance(x, Leaf) is better than hasattr("value").

http://codereview.appspot.com/2013042/diff/1/4#newcode210
lib2to3/btm_utils.py:210: name = name_leaf.value[1:][:-1]
You can use value.strip("\"") here.

http://codereview.appspot.com/2013042/diff/1/4#newcode227
lib2to3/btm_utils.py:227: #TODO: handle {min, max} repeaters
Perhaps raise NotImplementedError until that happens?

http://codereview.appspot.com/2013042/diff/1/4#newcode232
lib2to3/btm_utils.py:232: for child in details_node.children[1:][:-1]:
[1:-1] is better.

http://codereview.appspot.com/2013042/diff/1/4#newcode247
lib2to3/btm_utils.py:247: if type(subpatterns) is not list:
Use isinstance.

http://codereview.appspot.com/2013042/diff/1/4#newcode249
lib2to3/btm_utils.py:249: if type(subpatterns) is list and len(subpatterns)==1:
You don't need the type check here again.

http://codereview.appspot.com/2013042/diff/1/4#newcode261
lib2to3/btm_utils.py:261: lambda x: type(x) is str and x in common_chars)):
Use isinstance().

http://codereview.appspot.com/2013042/diff/1/4#newcode277
lib2to3/btm_utils.py:277: return sorted(subpatterns, key=len, reverse=True)[0]
Can you use max() instead?

http://codereview.appspot.com/2013042/diff/1/4#newcode283
lib2to3/btm_utils.py:283: if type(x) is list or type(x) is tuple:
isinstance(x, (list, tuple)) should work.

http://codereview.appspot.com/2013042/diff/1/8
File lib2to3/fixer_base.py (right):

http://codereview.appspot.com/2013042/diff/1/8#newcode68
lib2to3/fixer_base.py:68: self.pattern, self.pattern_tree = \
Split into several statements.

http://codereview.appspot.com/2013042/diff/1/2
File lib2to3/pytree.py (right):

http://codereview.appspot.com/2013042/diff/1/2#newcode219
lib2to3/pytree.py:219: if type(self) is Leaf:
How about overloading this function in the leaf class instead?

http://codereview.appspot.com/2013042/diff/1/2#newcode255
lib2to3/pytree.py:255: fixers_applied=[]):
You should use None as a default instead and create a list in the function body
if it is None.

http://codereview.appspot.com/2013042/diff/1/7
File lib2to3/refactor.py (right):

http://codereview.appspot.com/2013042/diff/1/7#newcode425
lib2to3/refactor.py:425: while any(list(match_set.values())):
The list() is redundant I believe.

http://codereview.appspot.com/2013042/diff/1/7#newcode427
lib2to3/refactor.py:427: if fixer in match_set.keys() and match_set[fixer]:
The keys() call is unneeded.

http://codereview.appspot.com/2013042/diff/1/7#newcode436
lib2to3/refactor.py:436: for node in list(match_set[fixer]):
Don't need the list().

http://codereview.appspot.com/2013042/diff/1/7#newcode467
lib2to3/refactor.py:467: if not fxr in list(match_set.keys()):
You don't need the list() or keys() call.

http://codereview.appspot.com/2013042/diff/1/7#newcode468
lib2to3/refactor.py:468: match_set[fxr]=[]
Add space.

[gboutsioukis, 2010-08-27 20:15:28]

Corrections & tests added

[gboutsioukis, 2010-08-27 20:16:16]

http://codereview.appspot.com/2013042/diff/1/8
File lib2to3/fixer_base.py (right):

http://codereview.appspot.com/2013042/diff/1/8#newcode68
lib2to3/fixer_base.py:68: self.pattern, self.pattern_tree = \
On 2010/08/20 19:41:12, Benjamin wrote:
> Split into several statements.

Splitting this statement introduces some overhead, as the pattern will have to
be compiled again. The best solution would be to split compile_pattern into two
methods, and feed the pattern tree to the second, but isn't some ugliness
preferable?

http://codereview.appspot.com/2013042/diff/1/2
File lib2to3/pytree.py (right):

http://codereview.appspot.com/2013042/diff/1/2#newcode221
lib2to3/pytree.py:221: 
On 2010/08/20 12:17:09, gboutsioukis wrote:
> The next two methods are actually not used in the final version and it's safe
to
> remove them.
Actually depth is used

http://codereview.appspot.com/2013042/diff/1/7
File lib2to3/refactor.py (right):

http://codereview.appspot.com/2013042/diff/1/7#newcode436
lib2to3/refactor.py:436: for node in list(match_set[fixer]):
On 2010/08/20 19:41:12, Benjamin wrote:
> Don't need the list().
That's actually an optimization

[Benjamin, 2010-08-27 20:31:22]

LGTM.

http://codereview.appspot.com/2013042/diff/1/8
File lib2to3/fixer_base.py (right):

http://codereview.appspot.com/2013042/diff/1/8#newcode68
lib2to3/fixer_base.py:68: self.pattern, self.pattern_tree = \
On 2010/08/27 20:16:16, gboutsioukis wrote:
> On 2010/08/20 19:41:12, Benjamin wrote:
> > Split into several statements.
> 
> Splitting this statement introduces some overhead, as the pattern will have to
> be compiled again. The best solution would be to split compile_pattern into
two
> methods, and feed the pattern tree to the second, but isn't some ugliness
> preferable?

What? Can't you write:

compiler = PatternCompiler()
self.pattern, self.pattern_tree = compiler.compile_pattern(self.PATTERN,
with_tree=True)

[gboutsioukis, 2010-08-28 01:26:34]

Patch3

[mdjr.1877, 2026-08-13 20:19:59]

On 2010/08/27 20:31:22, Benjamin wrote:
> <font dir="auto" style="vertical-align: inherit;"><font dir="auto"
style="vertical-align: inherit;">يبدو جيدًا.
</font>http://codereview.appspot.com/2013042/diff/1/8<font dir="auto"
style="vertical-align: inherit;"> 
> الملف lib2to3/fixer_base.py (على اليمين):
</font>http://codereview.appspot.com/2013042/diff/1/8#newcode68<font dir="auto"
style="vertical-align: inherit;"> 
> lib2to3/fixer_base.py:68: self.pattern, self.pattern_tree = \ </font><font
dir="auto" style="vertical-align: inherit;">
> في 2010/08/27 20:16:16، كتب gboutsioukis: </font><font dir="auto"
style="vertical-align: inherit;">
> > في 2010/08/20 19:41:12، كتب Benjamin: </font><font dir="auto"
style="vertical-align: inherit;">
> > > تقسيم إلى عدة عبارات. </font><font dir="auto" style="vertical-align:
inherit;">
> > </font><font dir="auto" style="vertical-align: inherit;">
> > يؤدي تقسيم هذه العبارة إلى بعض التكاليف الإضافية، حيث سيتعين </font><font
dir="auto" style="vertical-align: inherit;">
> إعادة تجميع النمط. الحل الأمثل هو تقسيم compile_pattern إلى </font><font
dir="auto" style="vertical-align: inherit;">
> طريقتين </font><font dir="auto" style="vertical-align: inherit;">
> ، وتمرير شجرة النمط إلى الثانية، ولكن أليس </font><font dir="auto"
style="vertical-align: inherit;">
> من الأفضل وجود بعض التعقيد؟ </font><font dir="auto" style="vertical-align:
inherit;">
> ماذا؟ ألا يمكنك كتابة: </font><font dir="auto" style="vertical-align:
inherit;">
> compiler = PatternCompiler() </font><font dir="auto" style="vertical-align:
inherit;">
> self.pattern, self.pattern_tree = compiler.compile_pattern(self.PATTERN,
</font><font dir="auto" style="vertical-align: inherit;">
> with_tree=True)</font></font>
> 
> http://codereview.appspot.com/2013042/diff/1/8<font dir="auto"
style="vertical-align: inherit;"></font>
> 
> http://codereview.appspot.com/2013042/diff/1/8#newcode68<font dir="auto"
style="vertical-align: inherit;"></font><font dir="auto" style="vertical-align:
inherit;"></font><font dir="auto" style="vertical-align: inherit;"></font><font
dir="auto" style="vertical-align: inherit;"></font><font dir="auto"
style="vertical-align: inherit;"></font><font dir="auto" style="vertical-align:
inherit;"></font><font dir="auto" style="vertical-align: inherit;"></font><font
dir="auto" style="vertical-align: inherit;"></font><font dir="auto"
style="vertical-align: inherit;"></font><font dir="auto" style="vertical-align:
inherit;"></font>
> <font dir="auto" style="vertical-align: inherit;"></font>
> <font dir="auto" style="vertical-align: inherit;"></font><font dir="auto"
style="vertical-align: inherit;"></font><font dir="auto" style="vertical-align:
inherit;"></font>