Enhancement of List Concatenation and Iterator Check #1393
Conversation
Effective List Concatenation: extend() was used in place of list concatenation (+) to improve efficiency. Simplified Iterator Check: Rather than using a bespoke method, the all() function was used to confirm that the iterator was empty. Improved Conditions: Replaced function calls with direct tests for alphanumeric characters and combined conditions directly for readability. Improved yield from: Made the yield from statement simpler for recursive calls, which made the code easier to read and understand.
Update reconstruct.py
There was a problem hiding this comment.
I am sorry, what is your goal with this PR? Many of the changes are either
- breaking potential usecases
- unnecessary
- or harder to read.
While I appreciate attempts to contribute to open source, it would be preferably if you didn't make random changes but instead tried to fix a few existing issues.
| @@ -50,18 +39,15 @@ def __default__(self, data, children, meta): | |||
| else: | |||
| x = next(iter_args) | |||
| if isinstance(x, list): | |||
| to_write += x | |||
| to_write.extend(x) | |||
There was a problem hiding this comment.
This changes shows that you do not know how python works. += for lists is exactly equivalent to .extend (probably faster since it's not a name lookup), see documentation.
| else: | ||
| assert NonTerminal(x.data) == sym, (sym, x) | ||
| assert (isinstance(x, Token) and Terminal(x.type) == sym) or \ | ||
| (isinstance(x, NonTerminal) and NonTerminal(x.data) == sym), (sym, x) |
There was a problem hiding this comment.
What exactly is the point of this change? This is just harder to read.
| to_write.append(x) | ||
|
|
||
| assert is_iter_empty(iter_args) | ||
| assert all(x is None for x in iter_args) |
There was a problem hiding this comment.
This is a change in behavior. Why?
| @@ -100,7 +85,7 @@ def reconstruct(self, tree: ParseTree, postproc: Optional[Callable[[Iterable[str | |||
| y = [] | |||
| prev_item = '' | |||
| for item in x: | |||
| if insert_spaces and prev_item and item and is_id_continue(prev_item[-1]) and is_id_continue(item[0]): | |||
| if insert_spaces and prev_item[-1:].isalnum() and item and item[0].isalnum(): | |||
There was a problem hiding this comment.
This is a change in behavior. Why?
| @@ -88,7 +74,6 @@ def _reconstruct(self, tree): | |||
| res = self.write_tokens.transform(unreduced_tree) | |||
| for item in res: | |||
| if isinstance(item, Tree): | |||
| # TODO use orig_expansion.rulename to support templates | |||
There was a problem hiding this comment.
Why delete this comment? Did you fix the TODO? Is the TODO no longer needed?
| @@ -1,6 +1,3 @@ | |||
| """This is an experimental tool for reconstructing text from a shaped tree, based on a Lark grammar. | |||
| """ | |||
|
|
|||
Sure, I will focus on existing issues.I wanted to make the function more clear but now realized that Its not working for some use cases |
1)Effective List Concatenation: extend() was used in place of list concatenation (+) to improve efficiency.
2)Simplified Iterator Check: Rather than using a bespoke method, the all() function was used to confirm that the iterator was empty.
3)Improved Conditions: Replaced function calls with direct tests for alphanumeric characters and combined conditions directly for readability.
4)Improved yield from: Made the yield from statement simpler for recursive calls, which made the code easier to read and understand.