Skip to content

Refactor related parameterized rule and inline rule - #536

Merged
ydah merged 12 commits into
ruby:masterfrom
ydah:refactor-parameterized
Feb 16, 2025
Merged

Refactor related parameterized rule and inline rule#536
ydah merged 12 commits into
ruby:masterfrom
ydah:refactor-parameterized

Conversation

@ydah

@ydah ydah commented Feb 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

The following refactoring was performed

  • Use "Parameterized" instead of "Parameterizing".
    • Reduce the number of feature name types and organize them a bit
  • Implement inline rule resolution with dedicated resolver class.

@ydah
ydah force-pushed the refactor-parameterized branch from 22d16d3 to 02eb9fa Compare February 10, 2025 23:30
@ydah ydah changed the title Use "Parameterized" instead of "Parameterizing" Refactor related parameterized rule and inline rule Feb 10, 2025
@ydah
ydah force-pushed the refactor-parameterized branch from 06ed102 to 851bd18 Compare February 13, 2025 13:45
@ydah

ydah commented Feb 13, 2025

Copy link
Copy Markdown
Member Author

Resolve conflicts

ydah added 11 commits February 14, 2025 23:36
@ydah
chore: rename parameterizing_rule to parameterized
b9f3bde
@ydah
refactor: enhance RBS signatures to rbs-inline style
3484241
@ydah
refactor: rename rhs_list to rhs for consistency
98c35ea
@ydah
refactor: rename add_parameterizing_rule to add_rule for clarity
4ae3b55
@ydah
refactor: rename insert_before_parameterizing_rules to prepend_parame…
adabb47 
…terized_rules for clarity
@ydah
refactor: rename parameterizing_rule to parameterized and update rela…
cb346d6 
…ted methods for consistency
@ydah
refactor: update parameterizing terminology to parameterized for cons…
3095413 
…istency
@ydah
refactor: update error message terminology from "Parameterizing rule"…
28be8d0 
… to "Parameterized rule" for consistency
@ydah
refactor: update terminology from "parameterizing rules" to "paramete…
b6ece6a 
…rized rules" for consistency in README
@ydah
refactor: implement inline rule resolution with dedicated resolver class
33ac52b
@ydah
refactor: update test descriptions from "parameterizing rule" to "par…
c4f1dab 
…ameterized rule" for consistency
@ydah
ydah force-pushed the refactor-parameterized branch from 851bd18 to c4f1dab Compare February 14, 2025 14:37
@ydah
ydah requested a review from yui-knk February 16, 2025 00:59
@yui-knk

yui-knk commented Feb 16, 2025

Copy link
Copy Markdown
Collaborator

I will review this PR.

@yui-knk

yui-knk commented Feb 16, 2025

Copy link
Copy Markdown
Collaborator

I agree to use "parameterized" in code base, actually "parameterized" is used in menhir code base.
One point need consideration is the name of this feature https://github.com/ruby/lrama/pull/536/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R45 because it's "parameterizing" when users define new parameterized rules.
Other seems good to me.

@ydah

ydah commented Feb 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

I agree to use "parameterized" in code base, actually "parameterized" is used in menhir code base. One point need consideration is the name of this feature https://github.com/ruby/lrama/pull/536/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R45 because it's "parameterizing" when users define new parameterized rules. Other seems good to me.

@yui-knk Indeed it is. We have reverted the function name to remain “Parameterizing” in fb02d43
When indicating the name of a function, “Parameterizing” in Upper CamelCase is used.

@yui-knk

yui-knk commented Feb 16, 2025

Copy link
Copy Markdown
Collaborator

LGTM

ydah
ydah commented Feb 16, 2025
View reviewed changes
Comment thread lib/lrama/warnings/redefined_rules.rb
grammar.parameterizing_rule_resolver.redefined_rules.each do |rule|
@logger.warn("parameterizing rule redefined: #{rule}")
grammar.parameterized_resolver.redefined_rules.each do |rule|
@logger.warn("parameterized rule redefined: #{rule}")

@ydah ydah Feb 16, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

MEMO) I had a little difficulty here. Since this is a warning against “parameterized rules” rather than function names, I did not use Parameterizing. I don't have any strong opinions on this, so I will revert back if it feels different.

@ydah
ydah merged commit fe80a27 into ruby:master Feb 16, 2025
@ydah
ydah deleted the refactor-parameterized branch February 16, 2025 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yui-knk yui-knk yui-knk approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ydah @yui-knk