-
Notifications
You must be signed in to change notification settings - Fork 665
MAINT: Standardize scenario VERSION constant and replace asserts with proper lifecycle checks #1355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…hecks in scenarios Co-authored-by: ValbuenaVC <50061128+ValbuenaVC@users.noreply.github.com>
Co-authored-by: ValbuenaVC <50061128+ValbuenaVC@users.noreply.github.com>
Co-authored-by: ValbuenaVC <50061128+ValbuenaVC@users.noreply.github.com>
|
|
||
| version: int = 1 | ||
| VERSION: int = 1 | ||
| version: int = 1 # Alias for backward compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Can you check if there's a way to use VERSION as the reference for the backwards-compatible alias? I don't think something like VERSION: int = 1; version = VERSION would work as expected, but give it (or something similar) a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Changed to version: int = VERSION which works correctly in Python. All tests passing. (4290950)
| raise ValueError( | ||
| "Scenario not properly initialized. Call await scenario.initialize_async() before running." | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Make sure that for each scenario you add the ValueError each scenario raises to the function's docstring(s) to enforce ruff compliance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ValueError documentation to all 6 affected function docstrings across the scenario files for ruff compliance. (4290950)
…trings Co-authored-by: ValbuenaVC <50061128+ValbuenaVC@users.noreply.github.com>
|
|
||
| version: int = 1 | ||
| VERSION: int = 1 | ||
| version: int = VERSION # Alias for backward compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should either print_deprecation_message when lowercase version is used, or we can likely just migrate. I think just migrating is fine since it's likely not used anywhere
If we print_deprecation_message, we should also check the unit test warnings to ensure everything is using the new VERSION
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably just get rid of version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree since it's really really new.
Scenarios were inconsistent: some used
version, othersVERSION. Additionally, lifecycle validation usedassert self._* is not None, which gets stripped in optimized mode (python -O), breaking production validation.Changes
VERSION(uppercase) as primary constant, with lowercaseversionalias that referencesVERSIONdirectly for backward compatibilityassert self._objective_target is not None→if self._objective_target is None: raise ValueError(...)is Nonecomparison to avoid false positives from empty collectionsExample
Affects 7 scenario classes across airt, garak, and foundry packages.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.