fix: raise ValueError for unrecognized strategy names in strategy_features() - #425
Conversation
…tures() strategy_features() silently accepted any unrecognized strategy name (e.g. a typo like 'in_procesing') by falling through to the bare 'return list(core)' branch. This gave a wrong-but-plausible result with no error, making bugs hard to trace. Replace the catch-all return with an explicit check for the three known core-only strategies, and raise ValueError for anything else. Add tests covering both a typo'd strategy name and an empty string. Fixes yakew7#418
|
@shauryagangrade is attempting to deploy a commit to the yashkewlani2020-gmailcom's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey @shauryagangrade, your first PR on Fair Code, that's awesome, thank you. This project is about making AI more accountable, and contributions like yours are what keep that work going. We'll review your changes shortly. If you haven't already, give the contributing guide a quick read: it covers how audits are structured and what we look for in a review. And if you're finding Fair Code useful or interesting, a star on the repo genuinely helps more people find it - no pressure, just appreciated. |
There was a problem hiding this comment.
🟡 Changes recommended
The updated core-only branch returns list(core) without deduplication, which can yield duplicate feature columns and inconsistent behavior vs the other branches.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes faircode.strategies.strategy_features() fail fast by raising a ValueError when given an unrecognized strategy name, preventing silent fall-through to an incorrect-but-plausible feature set.
Changes:
- Replace the previous catch-all fallback in
strategy_features()with an explicit known-strategy check plusValueErrorfor unknown inputs. - Add unit tests asserting
ValueErroris raised for an unknown strategy typo and for the empty string.
File summaries
| File | Description |
|---|---|
faircode/strategies.py |
Adds explicit membership check for core-only strategies and raises on unknown strategy names. |
tests/test_strategies.py |
Adds tests verifying unknown/empty strategy inputs raise ValueError. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
faircode/strategies.py introduces a top-level-indented if that will cause an IndentationError / break strategy_features() at runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…n to live numbers CONTRIBUTORS.md was missing nitishchauhan002 (#410, first merged PR) and shauryagangrade (#425, first merged PR) entirely, and AnayDhawan's #412 wasn't reflected in their existing entry - snapshot bumped to cover everything through #425. CHANGELOG.md gets matching "Fixed" entries under the version already shipping (2.2.0), including the ambiguous-JSON follow-up fix from the previous commit. METRICS.md's weekly row and badges, and README's Traction table, updated to live GitHub numbers (stars, forks, contributors, issues closed this week) pulled via `gh`. Countries/social reach left unchanged - no access to site analytics or Instagram/LinkedIn impressions to verify those.
Fixes #418
What
faircode/strategies.py::strategy_features()silently accepted any unrecognized strategy name. The finalreturn list(core)is a bareelsein practice - meant to handle exactlyunawareness_proxy_removal,in_processing, andpost_processing, but a typo like"in_procesing"fell through to the same branch and returned a wrong-but-plausible column set with no error.Change
Replaced the bare catch-all with an explicit membership check for the three known core-only strategies, and added
raise ValueError(f"unknown strategy: {strategy!r}")for anything else.Why it matters
While
faircode/benchmark.pyonly iterates the fixedSTRATEGIEStuple today,strategy_featuresis a public (no leading underscore) function that anyone can import directly from a notebook, script, or future extension. A typo'd name silently produced a plausible wrong answer instead of an error.Tests
Added two tests to
tests/test_strategies.py:test_strategy_features_raises_on_unknown_strategy- a typo'd name ("in_procesing") raisesValueErrortest_strategy_features_raises_on_empty_string- empty string raisesValueErrorAll 13 tests in
tests/test_strategies.pypass;ruff checkis clean on both touched files.