Centralize action-argument validation in modelapi#86
Merged
jrfaeder merged 3 commits intoMay 11, 2026
Conversation
Route `Action.__init__` (in `structs.py`) through a single
`ActionList.validate_action` method (added to `utils.py`) so that direct
construction of `Action` instances applies the same schema check as the
parser. Previously the inline validation in `Action.__init__` had two
bugs:
- For positional actions (`no_setter_syntax` / `square_braces`,
e.g. `setConcentration`, `quit`, `saveConcentrations`) the
`arg_dict[type]` lookup returns an empty list `[]`, so the
`len(valid_arg_list) > 0` guard skipped validation entirely —
any args were accepted.
- For unknown action types the error message was good, but it ran
AFTER setting `self.type`, leaving a half-constructed object alive
in the rare case someone catches the exception.
The new `validate_action` adds:
- positional actions store args as a dict with `None` values
(the canonical shape the parser emits) — keyword-shape dicts
passed by hand are rejected with a clear error.
- per-action positional arity (`positional_arity` table) catches
`Action("quit", {'"x"': None})` (too many) and
`Action("setConcentration", {'"A()"': None})` (too few).
- normal-type validation: arg names must be in `arg_dict[type]`
(existing behavior, just centralized).
`Action.__init__` is simplified to call `validate_action` and then
just track duplicate-arg warnings in a small loop.
No behavior change for parse-time construction: the parser already
emits the canonical positional-args-as-`{value: None}` shape, so the
stricter direct-construction path doesn't reject anything the parser
produces.
Resolved conflict by combining: - Updated import style (separate lines, shutil instead of distutils) - Added BNGParseError import needed for validate_action method
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.
Summary
Route `Action.init` (in `structs.py`) through a single `ActionList.validate_action` method (added to `utils.py`) so that direct construction of `Action` instances applies the same schema check as the parser.
The previous inline validation in `Action.init` had two bugs:
What the new validation enforces
No behavior change at parse time
The parser already emits the canonical positional-args-as-`{value: None}` shape, so the stricter direct-construction path doesn't reject anything the parser produces. Only direct `Action(...)` calls with malformed args see a new error (which is the point).
Test plan