refactor: split __init__.py into logical modules#14
Merged
Conversation
Owner
shaia
commented
Dec 5, 2025
shaia
commented
- Extract version detection to _version.py
- Extract C extension loading to _loader.py
- Keep init.py as clean public API (~63 lines vs 125)
- Add ExtensionNotBuiltError for better error handling
- Extract version detection to _version.py - Extract C extension loading to _loader.py - Keep __init__.py as clean public API (~63 lines vs 125) - Add ExtensionNotBuiltError for better error handling
There was a problem hiding this comment.
Pull request overview
This PR refactors the __init__.py file by extracting version detection and C extension loading logic into separate modules (_version.py and _loader.py), making the codebase more modular and maintainable. The refactoring reduces __init__.py from ~125 lines to ~63 lines while introducing a new ExtensionNotBuiltError exception for better error handling.
- Extracts version detection logic to
_version.pywith aget_version()function - Extracts C extension loading to
_loader.pywith aload_extension()function and newExtensionNotBuiltErrorexception - Simplifies
__init__.pyto focus on public API definition and namespace population
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| cfd_python/_version.py | New module containing get_version() that handles version detection from package metadata or C module with fallback to "0.0.0-dev" |
| cfd_python/_loader.py | New module containing load_extension() for C extension loading, ExtensionNotBuiltError custom exception, and _check_extension_exists() helper function |
| cfd_python/init.py | Refactored to use extracted modules; imports get_version() and load_extension(), maintaining same public API with cleaner structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+6
to
+26
| def get_version() -> str: | ||
| """Get package version from metadata or C module.""" | ||
| try: | ||
| from importlib.metadata import PackageNotFoundError, version | ||
|
|
||
| try: | ||
| return version("cfd-python") | ||
| except PackageNotFoundError: | ||
| pass | ||
| except ImportError: | ||
| pass | ||
|
|
||
| # Try C module version | ||
| try: | ||
| from . import cfd_python as _cfd_module | ||
|
|
||
| return getattr(_cfd_module, "__version__", "0.0.0") | ||
| except ImportError: | ||
| pass | ||
|
|
||
| return "0.0.0-dev" |
There was a problem hiding this comment.
The new get_version() function and load_extension() function introduce new behavior and error handling logic but lack direct test coverage. The existing tests in test_import_handling.py and test_module.py test the overall module behavior, but don't test the internal _version.py and _loader.py modules directly.
Consider adding tests that:
- Test
get_version()with mockedimportlib.metadatascenarios - Test
load_extension()directly with mocked extension states - Test
ExtensionNotBuiltErroris raised appropriately - Test
_check_extension_exists()with various file patterns
Comment on lines
+14
to
+100
| def _check_extension_exists() -> bool: | ||
| """Check if compiled extension files exist in package directory.""" | ||
| package_dir = os.path.dirname(__file__) | ||
| return any( | ||
| f.startswith("cfd_python") and (f.endswith(".pyd") or f.endswith(".so")) | ||
| for f in os.listdir(package_dir) | ||
| ) | ||
|
|
||
|
|
||
| def load_extension(): | ||
| """Load the C extension module and return exports. | ||
|
|
||
| Returns: | ||
| tuple: (exports_dict, solver_constants) | ||
|
|
||
| Raises: | ||
| ImportError: If extension exists but fails to load | ||
| ExtensionNotBuiltError: If extension is not built (dev mode) | ||
| """ | ||
| try: | ||
| from . import cfd_python as _cfd_module # noqa: F401 | ||
| from .cfd_python import ( | ||
| OUTPUT_CSV_CENTERLINE, | ||
| OUTPUT_CSV_STATISTICS, | ||
| OUTPUT_CSV_TIMESERIES, | ||
| OUTPUT_FULL_FIELD, | ||
| OUTPUT_PRESSURE, | ||
| OUTPUT_VELOCITY, | ||
| create_grid, | ||
| get_default_solver_params, | ||
| get_solver_info, | ||
| has_solver, | ||
| list_solvers, | ||
| run_simulation, | ||
| run_simulation_with_params, | ||
| set_output_dir, | ||
| write_csv_timeseries, | ||
| write_vtk_scalar, | ||
| write_vtk_vector, | ||
| ) | ||
|
|
||
| # Collect all exports | ||
| exports = { | ||
| # Simulation functions | ||
| "run_simulation": run_simulation, | ||
| "run_simulation_with_params": run_simulation_with_params, | ||
| "create_grid": create_grid, | ||
| "get_default_solver_params": get_default_solver_params, | ||
| # Solver functions | ||
| "list_solvers": list_solvers, | ||
| "has_solver": has_solver, | ||
| "get_solver_info": get_solver_info, | ||
| # Output functions | ||
| "set_output_dir": set_output_dir, | ||
| "write_vtk_scalar": write_vtk_scalar, | ||
| "write_vtk_vector": write_vtk_vector, | ||
| "write_csv_timeseries": write_csv_timeseries, | ||
| # Output type constants | ||
| "OUTPUT_PRESSURE": OUTPUT_PRESSURE, | ||
| "OUTPUT_VELOCITY": OUTPUT_VELOCITY, | ||
| "OUTPUT_FULL_FIELD": OUTPUT_FULL_FIELD, | ||
| "OUTPUT_CSV_TIMESERIES": OUTPUT_CSV_TIMESERIES, | ||
| "OUTPUT_CSV_CENTERLINE": OUTPUT_CSV_CENTERLINE, | ||
| "OUTPUT_CSV_STATISTICS": OUTPUT_CSV_STATISTICS, | ||
| } | ||
|
|
||
| # Collect dynamic SOLVER_* constants | ||
| solver_constants = {} | ||
| for name in dir(_cfd_module): | ||
| if name.startswith("SOLVER_"): | ||
| solver_constants[name] = getattr(_cfd_module, name) | ||
|
|
||
| return exports, solver_constants | ||
|
|
||
| except ImportError as e: | ||
| if _check_extension_exists(): | ||
| # Extension file exists but failed to load - this is an error | ||
| raise ImportError( | ||
| f"Failed to load cfd_python C extension: {e}\n" | ||
| "The extension file exists but could not be imported. " | ||
| "This may indicate a missing dependency or ABI incompatibility." | ||
| ) from e | ||
| else: | ||
| # Development mode - module not yet built | ||
| raise ExtensionNotBuiltError( | ||
| "C extension not built. Run 'pip install -e .' to build." | ||
| ) from e |
There was a problem hiding this comment.
The load_extension() function and _check_extension_exists() function introduce new error handling logic but lack direct test coverage. While test_import_handling.py tests similar behavior at the module level, these internal functions are not directly tested.
Consider adding tests that:
- Test
_check_extension_exists()with various directory states (no files, .so files, .pyd files, etc.) - Test
load_extension()behavior when extension import succeeds - Test
ExtensionNotBuiltErroris raised when extension doesn't exist - Test
ImportErroris raised with appropriate message when extension exists but fails to load
| return version("cfd-python") | ||
| except PackageNotFoundError: | ||
| pass | ||
| except ImportError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Suggested change
| except ImportError: | |
| except ImportError: | |
| # importlib.metadata not available; fallback to C module version below |
Let other ImportErrors (missing deps, ABI issues) propagate with their helpful error messages instead of being silently caught.
- TestVersionModule: version detection scenarios - TestLoaderModule: extension loading, error handling, file detection - TestModuleIntegration: verify __init__.py integration
Clarify why each exception is intentionally caught and ignored.
_cfd_module is used for dir() and getattr() calls below.
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.