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

Limit error message overriding #493

Merged
merged 2 commits into from Jul 29, 2023
Merged

Conversation

ksunden
Copy link
Contributor

@ksunden ksunden commented Jul 5, 2023

This is a continuation from the conversation in matplotlib/matplotlib#26152.

I'm opening this here to further the conversation and so we can talk in slightly more concrete terms.

This is based on my last suggestion in that thread where I suggested Forward references without a name do not override the error message.

Either one of these factors or another error message override flag would be sufficient for matplotlib's purposes, so if you feel that this could be broader or would prefer to introduce a more explicit test, then feel free to say so and I will try to do so.

@ksunden ksunden changed the title Limit error message overriding when Limit error message overriding Jul 5, 2023
@ksunden
Copy link
Contributor Author

ksunden commented Jul 15, 2023

Gentle ping @ptmcg, would like to move this conversation forward, happy to do more if that is wanted

@ptmcg
Copy link
Member

ptmcg commented Jul 16, 2023

I haven't forgotten. On vacation this week, will try to sneak in a few minutes during quiet time, otherwise, next week.

@ptmcg
Copy link
Member

ptmcg commented Jul 20, 2023

This change looks like you have narrowed the field down to being detected by a minimal condition, thanks! I'll look at it in more detail in my dev environment when I get back home in a few days.

Could you add some tests to test_unit.py for this please? Include some cases that are not strictly aligned with your usage (see how Forward is used elsewhere in the test cases or the examples). If you don't care to work with unittest (not everyone's favorite), just attach a small test file to this PR and I'll incorporate it into test_unit.py myself.

@ksunden
Copy link
Contributor Author

ksunden commented Jul 21, 2023

I've added a test which is a significantly pared down version of what we do in matplotlib (though does use the same mechanism for raising the more custom exception)

I do check as well, though, that if you set the name that the more default behavior returns.

Not fully sure what other test cases would be warranted, as this does trace both branches of the introduced conditional.

@ksunden
Copy link
Contributor Author

ksunden commented Jul 27, 2023

Gentle nudge on this one, we are preparing for a release and would like to fix up (or remove) pins on pyparsing/xfail tests based on the specific version of pyparsing (as everything works other than having less helpful error messages, so may not want to wholly ban mpl from installing with pyparsing 3.1.0)

@ptmcg ptmcg self-requested a review July 28, 2023 07:58
@ptmcg ptmcg merged commit 173bc16 into pyparsing:master Jul 29, 2023
13 checks passed
@ptmcg
Copy link
Member

ptmcg commented Jul 30, 2023

Released today in pyparsing 3.1.1

@ksunden
Copy link
Contributor Author

ksunden commented Jul 31, 2023

Thanks, @ptmcg!

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