Skip to content
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.

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

Code refactoring/denesting for core.py #521

Merged
merged 4 commits into from Nov 20, 2023
Merged

Conversation

InSyncWithFoo
Copy link
Contributor

This PR is a part of #519.

@InSyncWithFoo
Copy link
Contributor Author

@ptmcg Since you insisted on not replacing not not with bool(), I tried my best not to introduce any new functions/methods or even calls.

@ptmcg
Copy link
Member

ptmcg commented Nov 7, 2023

I am actually very appreciative of this effort on your part. Go ahead and do refactoring as you feel justified, just please keep running the unit tests to verify you haven't introduced a regression, and be mindful of parse-time performance (which is sometimes the rationale for inlining code instead of calling a function - function call overhead is high in Python, especially in a deeply recursive code like pyparsing.

@ptmcg
Copy link
Member

ptmcg commented Nov 19, 2023

This is an enourmous PR. I'm going to go through change-by-change and mark each as accepted/rework/rejected.

  • Early returns help in denesting, but early continues can lead to future bugs seeping in (when code is added at the end of a for loop, but omitted in an early continue).
  • When an early return has to duplicate a complex return value, this creates a code duplication that will need to be maintained.
  • Conversion to f-strings looks good. When using multiple consecutive strings to build up a string that won't fit on a single line, I've found I'm less likely to forget a separating space if I move it to the beginning of the continuation line instead of the end of the previous line. Please use this style.

pyparsing/core.py Outdated Show resolved Hide resolved
pyparsing/core.py Outdated Show resolved Hide resolved
pyparsing/core.py Outdated Show resolved Hide resolved
pyparsing/core.py Outdated Show resolved Hide resolved
pyparsing/core.py Outdated Show resolved Hide resolved
pyparsing/core.py Outdated Show resolved Hide resolved
pyparsing/core.py Outdated Show resolved Hide resolved
pyparsing/core.py Outdated Show resolved Hide resolved
pyparsing/core.py Outdated Show resolved Hide resolved
pyparsing/core.py Outdated Show resolved Hide resolved
pyparsing/core.py Outdated Show resolved Hide resolved
return self

def ignore(self, other) -> ParserElement:
if isinstance(other, Suppress):
Copy link
Member

Choose a reason for hiding this comment

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

Why not if isinstance(other, Suppress) and other not in self.ignoreExprs: ? Negative logic looks weird to me here.

pyparsing/core.py Outdated Show resolved Hide resolved
@ptmcg ptmcg merged commit 8c8332d into pyparsing:master Nov 20, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants