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

Don't raise UnusedElementError when used has more elems than Grammar #24

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

cdamours
Copy link
Contributor

Issue

Currently, create_grammar will check if all named Elements are used by comparing the used variable (which tracks the named elements found by _used_checker) with elems (which tracks the values assigned to the grammar). The comparison is done with an != operator, which will fail for states where there are more used elements than included element.

Why make this change?

This change would allow users to be assign Element names outside the grammar initialization.

I noticed this while trying to add a name to a Regex element. Normally, names are added by using a class variable assignment in Grammar, however I have a grammar that conditionally includes Regex Elements based on user input. There are many different Regexes that could be included in the grammar, so naming them makes the Result.expecting easier to understand.

Steps to reproduce

int_value = Regex(r"\d+")
int_value.name = "INT_VALUE"

float_value = Regex(r"\d+(\.\d+)?")
float_value.name = "FLOAT_VALUE"

choice = Choice(float_value, int_value)
create_grammar(choice)

This gives us the following state:

elems = {"START"}
used = {"FLOAT_VALUE", "INT_VALUE", "START"}

This fails the condition if used != elems: and raises the following exception:

pyleri.exceptions.UnusedElementError: Unused element(s) found: 

Conclusion

When int_value/float_value are included in the grammar, they are in used, but it is not in elems, so when initializing the grammar, this raises an UnusedElementError. I felt like the correct approach was to update the logic to only raise an UnusedElementError when there are Elements in elems that are not in used.

Let me know if there are other perspectives I should be considering. I'm not sure if it's intended for names to be manually set on Elements, but the only issue I saw was this condition, so I wanted to know if we should change it.

Great job on pyleri, btw. I love it.

Copy link
Member

@joente joente left a comment

Choose a reason for hiding this comment

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

I like the idea and agree with the purposed solution!
Thanks for the pull request!

@joente joente merged commit cb44e4e into cesbit:master Mar 26, 2024
4 checks passed
joente added a commit that referenced this pull request Mar 26, 2024
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