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

Refactoring to extract Lrama::Grammar::Symbols #364

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

ydah
Copy link
Collaborator

@ydah ydah commented Jan 28, 2024

No description provided.

@yui-knk
Copy link
Collaborator

yui-knk commented Feb 4, 2024

Thanks for the PR. I think separating Symbol generation code into Resolver makes Grammar clean. However class hierarchy of Symbols::Base, Symbols::Nterms and Symbols::Terms is difficult. And accessing terms and nterms with @states.terms.symbols or @states.nterms.symbols is not intuitive because in this context symbol is a set of terms and nterms.

Can I ask you to

  1. Move all Symbol generation code, including #fill_symbol_number and so on, into Resolver
  2. Remove Symbols::Base, Symbols::Nterms and Symbols::Terms
  3. Keep Grammar#terms and Grammar#tnerms both of them will be delegated to symbols_resolver

…ate processing to the resolver

* Move all Symbol generation code, including #fill_symbol_number and so on, into Resolver
* Remove Symbols::Base, Symbols::Nterms and Symbols::Terms
* Keep Grammar#terms and Grammar#tnerms both of them will be delegated to symbols_resolver
@ydah
Copy link
Collaborator Author

ydah commented Feb 5, 2024

Fix to remove Symbols:: Base, Symbols:: Nterms, and Symbols:: Terms to delegate processing to resolvers. What do you think? Please point out if I missed something.

lib/lrama/grammar.rb Outdated Show resolved Hide resolved
@yui-knk
Copy link
Collaborator

yui-knk commented Feb 6, 2024

Left one comment, other seems good.

@yui-knk yui-knk merged commit b0c29ea into ruby:master Feb 6, 2024
17 checks passed
@yui-knk
Copy link
Collaborator

yui-knk commented Feb 6, 2024

Thanks!

@ydah ydah deleted the refactor-symbol branch February 6, 2024 04:03
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