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

Rewrite parser generator #84

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

heckad
Copy link

@heckad heckad commented Feb 25, 2023

What does this pull request do?

This pull request changes what we consider successful parsing. Now we have the not_parsed marker (the name can be changed) which mark that rule can't parse input. This solves the problem with empty collections being interpreted as parsing failure and also allows you to drag None up. Unfortunately this is a very big change. Parsers generated by the previous version will not work. However, this solves all problems.

@heckad heckad force-pushed the fix-zero-or-more-problem-parsing branch from 220a632 to 5471713 Compare February 25, 2023 16:59
Copy link
Contributor

@0dminnimda 0dminnimda left a comment

Choose a reason for hiding this comment

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

Good attempt

Also, if you are using someone else's ideas or code it's nice to mention them

Comment on lines +19 to +24
@final
class NotParsedMarker(Enum):
marker = 0


not_parsed = NotParsedMarker.marker
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just use object() and it'll give the same effect
Also @final doesn't do a lot, It's better to just use Literal[not_parsed] for annotations

| ENDMARKER { None }
| ENDMARKER { [] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change was made?

Copy link
Author

Choose a reason for hiding this comment

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

None is now a valid result but we expect a list to be returned there.

Comment on lines -270 to +279
return not ok
return not_parsed if ok is not not_parsed else True
Copy link
Contributor

Choose a reason for hiding this comment

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

bool check is more than enough here and in positive_lookahead

Copy link
Author

Choose a reason for hiding this comment

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

No, False now a correct parsing result. For mark incorrect parsing result we always should use not_parsed.

@heckad
Copy link
Author

heckad commented Feb 28, 2023

Also, if you are using someone else's ideas or code it's nice to mention them

I don't use any another idea. I just improved my last attempt. Now None, False and any other object are correct parsing results.

@heckad
Copy link
Author

heckad commented Mar 14, 2023

@MatthieuDartiailh, please look at pull request.

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.

2 participants