Skip to content

Add warning output when using %require, which exists only for compatibility with Bison#478

Merged
ydah merged 3 commits intoruby:masterfrom
ydah:warn-require
Feb 15, 2025
Merged

Add warning output when using %require, which exists only for compatibility with Bison#478
ydah merged 3 commits intoruby:masterfrom
ydah:warn-require

Conversation

@ydah
Copy link
Member

@ydah ydah commented Nov 7, 2024

This PR adds a warning output when using %require, which exists only for compatibility with Bison.
see: https://www.gnu.org/software/bison/manual/html_node/Require-Decl.html

…bility with Bison

This PR adds a warning output when using %require, which exists only for compatibility with Bison.
see: https://www.gnu.org/software/bison/manual/html_node/Require-Decl.html
@ydah ydah requested a review from yui-knk February 13, 2025 15:04
@yui-knk
Copy link
Collaborator

yui-knk commented Feb 14, 2025

Right now, I don't have any concrete direction about %require, implement it or deprecate it.

@ydah
Copy link
Member Author

ydah commented Feb 15, 2025

@yui-knk Thank you feedback. If I had one concern, it would be that when considering Lrama not as a parser generator for only CRuby but as a Bison-compatible parser generator, users might mistakenly believe they can control it with %require. This could lead to misunderstandings, which was my motivation for bringing it up.

For example, how about the following message?

"currently, %require is simply valid as a grammar but does nothing."

@yui-knk
Copy link
Collaborator

yui-knk commented Feb 15, 2025

The new message seems fine to me because it only explains current status.

@ydah
Copy link
Member Author

ydah commented Feb 15, 2025

@yui-knk I updated message. Thank you.

logger = Lrama::Logger.new
allow(logger).to receive(:warn)
Lrama::Warnings.new(logger, false).warn(grammar, states)
expect(logger).not_to have_received(:warn).with("currently, %require is simply valid as a grammar but does nothing")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better to check logger#warn is not called regardless its message otherwise the test case will pass when different message is send.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. Thank you!

@ydah ydah merged commit 1122a19 into ruby:master Feb 15, 2025
22 checks passed
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