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

Guidance for warn_ungrouped_named_tokens_in_collection #111

Closed
zopieux opened this issue Jul 30, 2019 · 5 comments
Closed

Guidance for warn_ungrouped_named_tokens_in_collection #111

zopieux opened this issue Jul 30, 2019 · 5 comments

Comments

@zopieux
Copy link

zopieux commented Jul 30, 2019

Issue #110 (and others) are about warn_ungrouped_named_tokens_in_collection being particularly noisy. According to the release notes, this warning was added in the latest major release, and recently silenced by default.

Looking at the code, the only explanation for this new warning is:

flag to enable warnings when a results name is defined on a containing expression with ungrouped subexpressions that also have results names

I find this a bit unhelpful. Warnings are meant to notify developers that something's wrong with their code and they should feel bad, eg. deprecation notices. But for such a warning to be effective, the path to a sane, no-warning state must be easily identified so developers are compelled to fix their code.

Could we get an explanation on how best to modify our code to fix this warning? I currently have code that relies on the behavior of having "nested" setResultName. It's probably bad practice, but works fine for me. Here is a small albeit dumb code that reproduces the issue. How would you approach the fix?

import pyparsing as pp
pp.__diag__.warn_ungrouped_named_tokens_in_collection = True

num = pp.Word(pp.nums).setParseAction(lambda t: int(t[0]))
left = num.setResultsName('left')
right = num.setResultsName('right')

equality = (left + pp.Suppress('==') + right).setResultsName('eq')
inequality = (left + pp.Suppress('!=') + right).setResultsName('ne')


def parse(tokens):
    left = tokens.get('left')
    right = tokens.get('right')
    if tokens.get('eq') is not None:
        return left == right
    if tokens.get('ne') is not None:
        return left != right
    raise ValueError("unknown operation")


clause = (equality | inequality).setParseAction(parse)

print(clause.parseString("42 == 42"))
print(clause.parseString("42 == 43"))

# test.py:8: UserWarning: warn_ungrouped_named_tokens_in_collection:
# setting results name 'eq' on And expression collides with 'right' on contained expression
# [True]
# [False]
@ptmcg
Copy link
Member

ptmcg commented Jul 31, 2019

Absolutely! There are 4 new warning-style __diag__ switches in pyparsing now, but very little guidance on what to do when you get them (other than their self-explanatory names - well, they are self-explanatory to me!). I'll start with a page on the Wiki, which will allow me to get some content out quickly with easy edit cycle. As they solidify, we can then add them as docstrings on the switches themselves.

You may have gotten that warning if you are running the interim-released 2.4.1.1, which installs with several diag switches enabled. If you upgrade to 2.4.2, all these diagnostics are disabled by default, so your warning ugliness will go away. But your comments are still valid for when you wish to make use of these new switches.

Here is an example of an ungrouped named tokens in collection:

term = ppc.identifier | ppc.number
# this expression has a results name, and the expressions it
# contains also have results names
eqn = (term("lhs") + '=' + term("rhs"))("eqn")

eqn.runTests("""\
    a = 1000
    """)

The resulting output is:

diag_examples.py:11: UserWarning: warn_ungrouped_named_tokens_in_collection: setting results name 'eqn' on And expression collides with 'rhs' on contained expression
  eqn = (term("lhs") + '=' + term("rhs"))("eqn")

a = 1000
['a', '=', 1000]
- eqn: ['a', '=', 1000]
- lhs: 'a'
- rhs: 1000

Note that all the results names are at the same level, no hierarchy. If other expressions in this parser had 'lhs' or 'rhs' names, in similar ungrouped hierarchy, the 'lhs' and 'rhs' names would clash, and the default would be for only the last name to be reported.

The resolution for this warning is to Group eqn:

eqn = Group(term("lhs") + '=' + term("rhs"))("eqn")

Which gives this output:

a = 1000
[['a', '=', 1000]]
- eqn: ['a', '=', 1000]
  - lhs: 'a'
  - rhs: 1000

Now 'lhs' and 'rhs' are grouped under 'eqn', and would not be overwritten by other 'lhs' or 'rhs' names in other expressions.

@ptmcg
Copy link
Member

ptmcg commented Jul 31, 2019

Sorry - you asked a specific question, which I talked about at length but didn't actually answer.

This change should resolve the warning for you. Change:

equality = (left + pp.Suppress('==') + right).setResultsName('eq')
inequality = (left + pp.Suppress('!=') + right).setResultsName('ne')

to:

equality = Group(left + pp.Suppress('==') + right).setResultsName('eq')
inequality = Group(left + pp.Suppress('!=') + right).setResultsName('ne')

@ptmcg
Copy link
Member

ptmcg commented Jul 31, 2019

See the wiki page "Parser Development Tips"

@zopieux
Copy link
Author

zopieux commented Jul 31, 2019

Thanks a lot for that in-depth reply. I guess I'll start Group'ing my expressions more, from now on :-)

That's a great wiki page you wrote there. I would encourage you to create short-links to this page and put them inside your warnings. That's such a great experience to just click on the warning link and get a full explanation. Cheers!

@zopieux zopieux closed this as completed Jul 31, 2019
@ptmcg
Copy link
Member

ptmcg commented Jul 31, 2019

Excellent suggestion, thanks! I've also renamed that page to "Parser Debugging and Diagnostics" since the only general suggestion was "Write a BNF", so I'm going to move that part (when it gets written) to a separate "Writing Your Parser" or something page. Should make it easier to find this page when dealing with a diag warning.

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

No branches or pull requests

2 participants