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

Meaningful repr for ParserElements #423

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Conversation

djpohly
Copy link
Contributor

@djpohly djpohly commented Jul 1, 2022

See #416 for discussion.

Populated instances of Forward are represented only as "Forward(...)" to prevent infinite recursion. I don't imagine this changing.

Helper classes like _PendingSkip and _ErrorStop are not yet handled, and there is no special handling for Literal. I'll ask about these in the issue.

I held off on creating unit tests for the new implementations to let the devs decide whether __repr__() actually belongs in the test suite or not.

A couple of notes:
 - Populated instances of Forward are represented only as "Forward(...)"
   to prevent infinite recursion.
 - Helper classes like _PendingSkip and _ErrorStop are not yet handled.
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2022

Codecov Report

Merging #423 (5735734) into master (05b3aad) will increase coverage by 0.05%.
The diff coverage is 89.58%.

@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
+ Coverage   89.67%   89.72%   +0.05%     
==========================================
  Files          11       11              
  Lines        4028     4157     +129     
  Branches     1013     1043      +30     
==========================================
+ Hits         3612     3730     +118     
- Misses        217      223       +6     
- Partials      199      204       +5     
Impacted Files Coverage Δ
pyparsing/core.py 89.08% <89.58%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05b3aad...5735734. Read the comment docs.

@@ -2299,7 +2301,7 @@ def must_skip(t):
def show_skip(t):
if t._skipped.as_list()[-1:] == [""]:
t.pop("_skipped")
t["_skipped"] = "missing <" + repr(self.anchor) + ">"
t["_skipped"] = f"missing <{self.anchor}>"
Copy link
Member

Choose a reason for hiding this comment

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

Used to show repr(self.anchor) - should change to {self.anchor!r}"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was so that an existing unit test would pass. I'm still hazy on the what/why of SkipTo in this context - should the test be changed instead?

@ptmcg
Copy link
Member

ptmcg commented Jul 3, 2022

Does this make the railroad diagrams better? worse?

@ptmcg
Copy link
Member

ptmcg commented Jul 3, 2022

If you are using features (such as typing annotations) not yet available until Py 3.7, I'll be dropping Py3.6 support in pyparsing 3.1, but this won't be for a few months yet.

@djpohly
Copy link
Contributor Author

djpohly commented Jul 3, 2022

Looks like railroad diagrams must use str() - the ones generated by the tests, at least, were identical before and after the changes.

IIRC everything worked fine with tox -e py36. Did I miss something?

Introduces a _make_repr() method with parameters to account for the
different contexts in which an element may need to be represented (top
level or as a term in an expression, as a potential string literal or
not).  In order to make the code a little easier to read, the
__format__() method is implemented as well, with conversion specifiers
used to indicate the context.
This function determines how to represent a set of characters using
srange(), then returns either the original set or the srange expression,
whichever is shorter.  To be used in repr() implementations in elements,
such as Word, which are based on sets of characters.
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

3 participants