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

AND/OR precedence is reversed compared to the original paper #199

Open
lonnen opened this issue Mar 30, 2022 · 4 comments · May be fixed by #208
Open

AND/OR precedence is reversed compared to the original paper #199

lonnen opened this issue Mar 30, 2022 · 4 comments · May be fixed by #208

Comments

@lonnen
Copy link
Collaborator

lonnen commented Mar 30, 2022

Parsimonious reverses the precedence of AND/OR compared to the original paper and other PEG libs

#191 (comment)

@lucaswiman
Copy link
Collaborator

(Copied from #191)

It's also been a tricky bug to fix, even when Erik was actively developing this lib. I don't think it is prudent to maintain both behaviors for the sake of backwards compat with pre-1.0 versions of Parsimonious.

Totally disagree with this. Unless the upgrade path is extremely easy and foolproof (like a function that converts an old grammar string to an equivalent new one), this would be breaking backwards compatibility for pretty questionable reasons: complying with some other parsers used by other people who aren't already using the library.

As a user of (and contributor to) parsimonious, with many functional grammar files, what other libraries are doing isn't very relevant unless it give some genuine functionality improvements. "pre-1.0" is sort of weak, since it is used in a lot of production systems.

The docs are pretty explicit. The README says:

I don't plan on making any backward-incompatible changes to the rule syntax in the future, so you can write grammars with confidence.

The comments on the grammar definition in the code says this:

# The grammar for parsing PEG grammar definitions:
# This is a nice, simple grammar. We may someday add to it, but it's a safe bet
# that the future will always be a superset of this.
rule_syntax = (r'''

It does seem like supporting both syntaxes shouldn't be that bad. Ideally the changes would just be to the grammar, though it looks like master...lower-precedence-ors required some changes to the visitor as well. Maybe a transducer from the old syntax to the new syntax would be possible, or at least an interesting exercise.

That said - there's still no clear owner for the issue. If someone is interested, though, it would be a high utility improvement for Parismonious!

Now that python 2 support has been dropped 🙌 , I'm personally more interested in working on allowing parsing of bytes objects. However, I'd be very interested in helping to review / test changes to syntax or precedence. I'm very glad that this library is getting more development velocity now that you have commit access!

@lonnen
Copy link
Collaborator Author

lonnen commented Mar 30, 2022

Cool, thanks!

@lucaswiman lucaswiman linked a pull request Apr 20, 2022 that will close this issue
2 tasks
@alexchandel
Copy link

"I don't plan on making any backward-incompatible changes to the rule syntax in the future, so you can write grammars with confidence" was added to the README in version 0.2, 12 years ago. Then he promptly shipped two breaking version (0.5, 0.6). It's sometimes necessary.

@beepbopbeepboop
Copy link

Rather than freeze out progress for time measured in years, why not have a state flag for new versus old. Default to 0, 0 meaning what it always meant, and 1 meaning, the new stuff, then in 100 years, one can flip it to 1, and break, or let users do version: 1, or some other way to select which one. feature-5: 1, for a specific feature. version: for the whole ball of wax. This let's you api update, if you need to, syntax changes, semantic changes, hide new additions, in short, anything. Longer term, yes, supporting a standard spelling that everyone agrees on, would be nice.

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 a pull request may close this issue.

4 participants