Restructure _utils#41
Merged
Merged
Conversation
_utils.py mixed four unrelated concerns. Redistribute its functions to
homes that match what they do, and delete the module:
search_ast_type -> _expressions.py (AST helpers)
to_cpp_name/to_camel_case/
generate_header_guard -> _names.py (C++ identifier naming)
get_compartment_size/
get_function_definition_args -> _sbml_reader.py (renamed from
_sbml_loader.py: SBML loading + readers)
Naming tests move alongside (test_utils.py -> test_names.py). Also drop the
resolved "Add priority" TODO in _format_events and correct the trigger
distance multi-operand comment.
Pure move: reference outputs regenerate unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add direct tests for the pure helpers that previously had only incidental
coverage:
_expressions: collect_ast_names, substitute_ast_names,
replace_avogadro_csymbol, strip_ast_units, search_ast_type
_sbml_reader: load_sbml_model (valid + read-error), get_compartment_size
(set + default), get_function_definition_arguments
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add assertion-based tests for two logic-heavy ModelBuilder paths that were
previously only verified by the byte-for-byte semantic-case diff:
_event_trigger_distance: all six relational operators (only < appears in
the reference models) plus the non-relational constant case
_compensate_compartment_resizes: direct resize, indirect resize via an
assignment rule, scaling an existing species assignment, and skipping
amount-tracked species
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR restructures internal “utility” functionality into more cohesive modules, splitting SBML-reading helpers, name/identifier utilities, and AST-expression helpers into their respective components. It aligns with the goal in #35 by consolidating identifier-derivation utilities alongside the name-conflict management code.
Changes:
- Removed
chaste_sbml/_utils.pyand redistributed functionality into_sbml_reader.py,_names.py, and_expressions.py. - Updated imports across the package (including reference-model tooling) to use the new module locations.
- Reorganized and expanded tests: moved name/header-guard tests into
test_names.py, addedtest_sbml_reader.py, and added more coverage around expression AST utilities and event-related ModelBuilder behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| chaste_sbml/tests/test_utils.py | Removed; its coverage is migrated to more specific test modules. |
| chaste_sbml/tests/test_sbml_reader.py | New tests for SBML loading and small SBML-element reader helpers. |
| chaste_sbml/tests/test_names.py | Now also tests generate_header_guard, to_camel_case, and to_cpp_name from _names. |
| chaste_sbml/tests/test_model_builder.py | Adds unit coverage for event-trigger distance formatting and compartment-resize compensation behavior. |
| chaste_sbml/tests/test_expressions.py | Adds tests for new/expanded AST helper functions (name collection/substitution, unit stripping, type search, avogadro handling). |
| chaste_sbml/SbmlRefModels/generate_cases.py | Updates import to generate_header_guard from _names. |
| chaste_sbml/chaste_sbml_model.py | Updates imports and docstring reference from _sbml_loader/_utils to _sbml_reader/_names. |
| chaste_sbml/_utils.py | Deleted; prior mixed responsibilities are moved into focused modules. |
| chaste_sbml/_sbml_reader.py | Now contains SBML file loading plus small “reader” helpers previously in _utils. |
| chaste_sbml/_names.py | Now contains string-to-C++ identifier utilities (camel/cpp/header guard) alongside NameManager logic. |
| chaste_sbml/_model_builder.py | Swaps SBML helper imports to _sbml_reader and updates inline TODO/comments. |
| chaste_sbml/_expressions.py | Inlines search_ast_type (previously from _utils) into the expressions module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Correct hints that contradicted the code and annotate value-returning
functions that lacked a return type:
search_ast_type / collect_ast_names roots -> Optional[ASTNode] (both
guard against None; noted in review)
Equation.math -> Optional[ASTNode] (_format_equations guards `if eq.math`)
load_sbml_model -> Model
_species_conversion_factor -> Optional[str] (returns an SId, not a float)
_add_species_initial_value -> Optional[float]
_event_priority -> Optional[str]
parse_args -> argparse.Namespace
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kwabenantim
marked this pull request as ready for review
July 5, 2026 09:56
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #41 +/- ##
===========================================
+ Coverage 91.93% 92.72% +0.78%
===========================================
Files 55 54 -1
Lines 6364 6361 -3
===========================================
+ Hits 5851 5898 +47
+ Misses 513 463 -50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Supports #35