Conversation
There was a problem hiding this comment.
Pull request overview
Updates RailsAdmin’s configurable option registration to avoid in-place mutation of option_name, addressing Ruby warnings about future freezing of strings returned by Symbol#to_s.
Changes:
- Replace
chop!(mutating) withchop(non-mutating) when handling boolean option names ending in?.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| option_name = option_name.to_s | ||
| options[option_name] = nil | ||
|
|
||
| # If it's a boolean create an alias for it and remove question mark | ||
| if option_name.end_with?('?') | ||
| scope.send(:define_method, "#{option_name.chop!}?") do | ||
| option_name = option_name.chop | ||
| scope.send(:define_method, "#{option_name}?") do |
There was a problem hiding this comment.
Dear Copilot, thanks for your valued suggestion, but your suggestion introduces a bug that previously options[original_name] was set to nil while you're setting options[normalized_name] to nil (which wasn't there anyway). As in the original version as well as in my change, the option_name is no longer in the options hash, there's no need to change has_option?. Changing the name to normalized_name through the whole method makes the previously isolated special case of boolean getters bleed much further. Thus, I'd prefer to leave it as is.
| # If it's a boolean create an alias for it and remove question mark | ||
| if option_name.end_with?('?') | ||
| scope.send(:define_method, "#{option_name.chop!}?") do | ||
| option_name = option_name.chop | ||
| scope.send(:define_method, "#{option_name}?") do | ||
| send(option_name) | ||
| end |
There are a lot of warnings about a string "will be frozen in the future" (depending on the users code base).
E.g.
This fixes it by not mutating the option_name string inline but explicitly reassigning.