Skip to content

Add found string as property #571

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

Jump to bottom
Merged
merged 3 commits into from
Sep 23, 2024
Merged

Add found string as property #571

merged 3 commits into from
Sep 23, 2024

Conversation

OdysseasKr
Copy link
Contributor

A bit of a silly change, feel free to close if you don't accept these.

I have moved the logic of the "found" string out of the __str__ magic method. This allows the user to generate prettier messages from the exception. I also think it's a bit cleaner to avoid parsing logic in __str__

Example

From the following exception

In [2]: Keyword("KEY1").parse_string("KEY2", parseAll=True)
---------------------------------------------------------------------------
ParseException                            Traceback (most recent call last)
Cell In[2], line 1
----> 1 Keyword("KEY1").parse_string("KEY2", parseAll=True)

File ~/Documents/pyparsing/pyparsing/core.py:1197, in ParserElement.parse_string(self, instring, parse_all, parseAll)
   1194         raise
   1195     else:
   1196         # catch and re-raise exception from here, clearing out pyparsing internal stack trace
-> 1197         raise exc.with_traceback(None)
   1198 else:
   1199     return tokens

ParseException: Expected Keyword 'KEY1', found 'KEY2'  (at char 0), (line:1, col:1)

I would like to generate a message like this

1:1 Expected Keyword 'KEY1', found 'KEY2'

With the existing implementation the user would have to call __str__ and then remove the character, line and column information.

@OdysseasKr
Add found str as property
674da1c 
Moved logic about the "found" string from __str__ to its own property in
`ParseBaseException`.
@OdysseasKr OdysseasKr changed the title Add found str as property Add found string as property Sep 22, 2024
@ptmcg
Copy link
Member

ptmcg commented Sep 22, 2024

I'll give it a look. There are a few very popular projects that use pyparsing and rely on the specific format of exception messages, so I try to keep these messages from changing if I can help it.

Adding a new property should not be a problem, and with that you might be able to monkeypatch ParseBaseException.__str__ to reformat it to your liking.

And those projects might also benefit from this new property, so there is that possibililty as well (though pyparsing elements of most projects are often in the category of "that one guy wrote this 10 years ago, so let's not ever change it").

ptmcg
ptmcg reviewed Sep 22, 2024
View reviewed changes
@ptmcg
Copy link
Member

ptmcg commented Sep 22, 2024

Ok, I think this would make for a nice change, and there are likely some (slight) performance improvements we can do, too. See my review comment.

@ptmcg
Copy link
Member

ptmcg commented Sep 23, 2024

Or if you'd rather, I can accept your PR as-is, and then make these adjustments afterward (and also add tests and CHANGES note).

@OdysseasKr
Copy link
Contributor Author

OdysseasKr commented Sep 23, 2024 •
edited
Loading

Makes sense, should have used cached_property for this. I will make the changes today!

@OdysseasKr
Cache computed properties
3be1ad5 
Use `cached_property` to prevent re-computing properties of `ParseBaseException`
@OdysseasKr
Move formatted message logic to separate method
7b143b6
@OdysseasKr OdysseasKr requested a review from ptmcg September 23, 2024 12:26
@ptmcg ptmcg self-assigned this Sep 23, 2024
ptmcg
ptmcg approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@ptmcg ptmcg left a comment

Choose a reason for hiding this comment

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

Looks good to me, merging

@ptmcg ptmcg merged commit eed38b0 into pyparsing:master Sep 23, 2024
8 checks passed
@richardotis richardotis mentioned this pull request Nov 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptmcg ptmcg ptmcg approved these changes

Assignees

@ptmcg ptmcg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@OdysseasKr @ptmcg