#481: preserve true bool PMP defaults#487
Conversation
akmhatey-ai
left a comment
There was a problem hiding this comment.
Checked the current head b85f0855194d6fad4c307eccf48d41e6963f71b1 against #481.
The one-line change is scoped to the failing path: an XML PMP bool default is already coerced to Ruby true, and the final bool conversion now accepts it via result.to_s == 'true'. Existing string overrides still behave as before ('true' stays true, 'false' stays false), and the new regression test covers the no-factbase-override/default-true case from the issue.
Validation I ran locally:
bundle installwith isolatedBUNDLE_PATHpassed.- Focused PMP tests passed: 9 tests, 17 assertions, 0 failures/errors/skips.
bundle exec rubocop lib/fbe/pmp.rb test/fbe/test_pmp.rb --format simplepassed.- Full
bundle exec rubocop --format simplepassed: 65 files, no offenses. git diff --check origin/master...HEADpassed.- Diff-only
gitleaks stdin --no-banner --redact --exit-code 1passed with no findings.
One local limitation: full bundle exec rake test on my Windows checkout hit unrelated environment errors around SQLite temp DB cleanup permissions and /bin/bash not existing. The touched TestPmp coverage passes locally, and the hosted Ubuntu/macOS rake checks are green, so I do not see that as caused by this PR.
|
@akmhatey-ai Hey! Nice work on that review 👍 You earned +10 points this time: started with the standard +18 base points, but had to deduct 8 since there weren't any comments posted during the review. Next time, try leaving a few comments to avoid that deduction and boost your score even higher! Your running total is now +119 - keep it up and don't forget to check your Zerocracy account too. |
|
Please fix the merge conflicts so this pull request can be merged. |
morphqdd
left a comment
There was a problem hiding this comment.
Correct fix for #481 — result == 'true' returns false when result is already the boolean true (true == 'true' is false), so a bool PMP with a true default was silently flipped to false. result.to_s == 'true' handles both the string-from-XML and the already-boolean cases. The test with a <default>true</default> bool covers it. LGTM.
| when 'int' then Integer(Float(result).truncate) | ||
| when 'float' then Float(result) | ||
| when 'bool' then result == 'true' | ||
| when 'bool' then result.to_s == 'true' |
There was a problem hiding this comment.
Good — .to_s normalizes both paths: 'true' (parsed from XML) and a boolean true (when the default is already typed) both yield truthy, while false/'false'/anything else stays false. Handles the exact case #481 describes.
|
@morphqdd Thanks for the review! You've earned +13 points for this: +18 as a basis; -5 for very few (1) comments. Your running score is +778; don't forget to check your Zerocracy account too). |
Summary:
Closes #481
Validation:
No secrets were added.