multi(core): Various fixes to the Settings system#465
Merged
Conversation
405faf8 to
d76a3c9
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves the robustness and safety of Toys::Settings type validation and data loading, adding guards for edge cases (e.g., inherited fields, YAML loading, and stricter conversions) and expanding test coverage to lock in the behavior.
Changes:
- Add reserved settings field name blocking and improve type matching/conversion behavior (ranges, regexps, unions, numeric parsing).
- Fix settings field inheritance for subclasses and propagate nested-group loading errors correctly (including
raise_on_failurebehavior). - Switch YAML loading to
Psych.safe_load(disallowing arbitrary Ruby object deserialization) while permitting symbols.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| toys-core/lib/toys/settings.rb | Core fixes: reserved names, safer YAML loading, improved converters/type matching, inherited fields, and nested error propagation. |
| toys-core/test/test_settings.rb | Adds regression tests for reserved names, subclass field inheritance, stricter conversions, union prioritization, ranges, YAML safety, and nested error propagation. |
Comments suppressed due to low confidence (1)
toys-core/lib/toys/settings.rb:910
interpret_nameonly checkspublic_instance_methods(false)for duplicates, so with the new inheritedfieldsbehavior a subclass can re-declare a field name that already exists in the superclass without raising, silently overwriting the inherited field definition and accessor methods. Consider also checkingfields.key?(name.to_sym)and/or usingpublic_instance_methods(including inherited) so redefinitions are rejected consistently across inheritance.
def interpret_name(name)
name = name.to_s
if name !~ /^[a-zA-Z]\w*$/ || RESERVED_FIELD_NAMES.include?(name)
raise ::ArgumentError, "Illegal settings field name: #{name}"
end
existing = public_instance_methods(false)
if existing.include?(name.to_sym) || existing.include?(:"#{name}=") ||
existing.include?(:"#{name}_set?") || existing.include?(:"#{name}_unset!")
raise ::ArgumentError, "Settings field already exists: #{name}"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fix(core): Block certain reserved names from being used as Settings field names fix(core): Fixed Settings#load_data! when subclassing another settings class fix(core): Reject non-String values in Settings regexp type spec fix(core): Settings guards against unknown classes when loading YAML on older Ruby versions fix(core): Settings guards against ILLEGAL_VALUE being passed to range.member? fix(core): Settings does a better job choosing exact-match values when using a union type fix(core): Settings reject non-numeric strings in Integer and Float converters fix(core): Settings propagates nested group errors in Settings#load_data! Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
d76a3c9 to
654bac0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(core): Block certain reserved names from being used as Settings field names
fix(core): Fixed Settings#load_data! when subclassing another settings class
fix(core): Reject non-String values in Settings regexp type spec
fix(core): Settings guards against unknown classes when loading YAML on older Ruby versions
fix(core): Settings guards against ILLEGAL_VALUE being passed to range.member?
fix(core): Settings does a better job choosing exact-match values when using a union type
fix(core): Settings reject non-numeric strings in Integer and Float converters
fix(core): Settings propagates nested group errors in Settings#load_data!