Refer to "dangerous" rather than "broken" options#1224
Conversation
…ken" options In various places in the code and documentation we talk about broken options being gated by the `please_give_me_broken_results` parameter. Strictly speaking, it is not true that all these hidden options are broken. For example, the `remaining_demand_absolute_tolerance` parameter is not broken: the reason for hiding it is that it is potentially dangerous for users to change it. Change variable names, comments etc. to talk about "dangerous" rather than "broken" options, where appropriate. I have left the name of the `please_give_me_broken_results` parameter the same, as enabling any of these options may lead to broken (or nonsensical) _results_, even if the option itself is not broken. This is mostly a developer-facing change, but I have also updated the language used in the schema.
alexdewar
force-pushed
the
broken-options-naming
branch
from
319d70a to
a5fb37c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1224 +/- ##
=======================================
Coverage 89.74% 89.74%
=======================================
Files 57 57
Lines 8195 8195
Branches 8195 8195
=======================================
Hits 7355 7355
Misses 544 544
Partials 296 296 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates internal naming and user/developer-facing messaging to refer to “dangerous” (rather than “broken”) gated options that are unlocked via please_give_me_broken_results, and reflects that wording in the input schema.
Changes:
- Renames the model-parameter flag/exports from “broken” to “dangerous” and updates associated docs/log/error messages.
- Updates gating checks/messages for experimental scarcity pricing.
- Updates the
model.yamlschema description forplease_give_me_broken_resultsto better describe intended usage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/model/parameters.rs | Renames the config flag and global gating mechanism; updates validation error/warn messaging and related tests. |
| src/model.rs | Updates public re-exports to the new “dangerous” names. |
| src/input/commodity.rs | Updates scarcity pricing gatekeeping to use the renamed gating function/constant and adjusts messaging. |
| schemas/input/model.yaml | Clarifies schema documentation for please_give_me_broken_results as enabling potentially dangerous/experimental options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "The 'scarcity' pricing strategy is currently experimental. \ | ||
| To run anyway, set the {ALLOW_BROKEN_OPTION_NAME} option to true." | ||
| To run anyway, set the {ALLOW_DANGEROUS_OPTION_NAME} option to true." |
There was a problem hiding this comment.
This \-continued multi-line error string includes indentation before “To run anyway…”, which will become leading whitespace in the emitted error message. Consider removing the indentation after the continuation (or otherwise building the string) so the user-facing message is formatted cleanly.
There was a problem hiding this comment.
That's not actually true; I just checked.
It's odd that Copilot trips over on some quite normal Rust constructs.
| "Setting a remaining_demand_absolute_tolerance different from the default value of \ | ||
| {:e} is potentially dangerous, set {ALLOW_DANGEROUS_OPTION_NAME} to true if you want \ | ||
| to allow this.", |
There was a problem hiding this comment.
These multi-line string literals use \ line continuation with indentation on the following lines, which becomes literal leading whitespace in the runtime error message. That makes the message harder to read and makes tests that match on message fragments more brittle; consider restructuring the string (e.g., no indentation after \, or build it without injected spaces).
| "Setting a remaining_demand_absolute_tolerance different from the default value of \ | |
| {:e} is potentially dangerous, set {ALLOW_DANGEROUS_OPTION_NAME} to true if you want \ | |
| to allow this.", | |
| "Setting a remaining_demand_absolute_tolerance different from the default value of " | |
| "{:e} is potentially dangerous, set {ALLOW_DANGEROUS_OPTION_NAME} to true if you want " | |
| "to allow this.", |
Description
In various places in the code and documentation we talk about broken options being gated by the
please_give_me_broken_resultsparameter. Strictly speaking, it is not true that all these hidden options are broken. For example, theremaining_demand_absolute_toleranceparameter is not broken: the reason for hiding it is that it is potentially dangerous for users to change it.Change variable names, comments etc. to talk about "dangerous" rather than "broken" options, where appropriate. I have left the name of the
please_give_me_broken_resultsparameter the same, as enabling any of these options may lead to broken (or nonsensical) results, even if the option itself is not broken.This is mostly a developer-facing change, but I have also tweaked the schema.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks