Repository navigation
Conversation
…Error The Versions.released property and Versions.check() method failed with ModuleNotFoundError because pysal/base.py imported from .frozen which did not exist in the package. Changes: - Add pysal/frozen.py with frozen_packages that falls back to installed versions when explicit frozen versions are not available - Add tests for Versions.released property and Versions.check() method
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #1397 where Versions.released property and Versions.check() method failed with ModuleNotFoundError because the pysal.frozen module did not exist in the package.
Changes:
- Added
pysal/frozen.pymodule that provides afrozen_packagesdictionary with fallback to installed versions - Added comprehensive tests for
Versions.releasedproperty andVersions.check()method to prevent regression
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pysal/frozen.py | New module providing frozen package versions with fallback to installed versions when explicit frozen data is not available |
| pysal/tests/test_base.py | Added tests for Versions.released property and Versions.check() method to verify they work without raising exceptions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| v = Versions() | ||
| try: | ||
| released = v.released | ||
| assert isinstance(released, dict) | ||
| except ModuleNotFoundError as e: | ||
| if "pysal.frozen" in str(e): | ||
| raise AssertionError( | ||
| "Versions.released raised ModuleNotFoundError for pysal.frozen. " | ||
| "The frozen module is missing from the package distribution." | ||
| ) from e | ||
| raise |
There was a problem hiding this comment.
Test complexity: The try-except blocks add unnecessary complexity. Since the frozen module is now included in the PR, the ModuleNotFoundError should never occur. The test can be simplified to just assert that v.released returns a dict without the try-except wrapper. If the module is missing, pytest will provide a clear error message.
| def _get_frozen_packages(): | ||
| """ | ||
| Get the frozen package versions. | ||
|
|
||
| For development installations or when frozen versions are not explicitly | ||
| set, this falls back to using the installed versions. This ensures that | ||
| Versions.released and Versions.check() work without raising errors. | ||
|
|
||
| Returns | ||
| ------- | ||
| dict | ||
| Dictionary mapping package names to version strings. | ||
| """ | ||
| versions = {} | ||
| for package in memberships: | ||
| versions[package] = _installed_version(package) | ||
| return versions | ||
|
|
||
|
|
||
| frozen_packages = _get_frozen_packages() |
There was a problem hiding this comment.
Missing integration with build process: This fallback implementation always returns installed versions, which means Versions.released will always equal Versions.installed. The tools/frozen.py script should be updated to generate this module with actual frozen version data during the release process. Consider adding a check that detects when frozen_packages contains actual release data vs. fallback data, and document that this file should be regenerated by tools/frozen.py during releases.
| "The frozen module is missing from the package distribution." | ||
| ) from e | ||
| raise | ||
|
|
There was a problem hiding this comment.
Missing direct test coverage: While the frozen module is tested indirectly through test_versions_released_property_does_not_raise and test_versions_check_does_not_raise, there are no direct tests for the _get_frozen_packages function or the frozen_packages variable. Consider adding tests that directly verify: (1) frozen_packages is a dictionary, (2) it contains all packages from memberships, and (3) all values are strings.
| v = Versions() | ||
| try: | ||
| v.check() | ||
| captured = capsys.readouterr() | ||
| assert "Package" in captured.out | ||
| assert "Installed" in captured.out | ||
| assert "Released" in captured.out | ||
| except ModuleNotFoundError as e: | ||
| if "pysal.frozen" in str(e): | ||
| raise AssertionError( | ||
| "Versions.check() raised ModuleNotFoundError for pysal.frozen. " | ||
| "The frozen module is missing from the package distribution." | ||
| ) from e | ||
| raise |
There was a problem hiding this comment.
Test complexity: Similar to test_versions_released_property_does_not_raise, this try-except block adds unnecessary complexity. The test can be simplified to just call v.check() and verify the output without the try-except wrapper. If the frozen module is missing, pytest will provide a clear error message.
fixes #1397.
The Versions.released property and Versions.check() method failed with ModuleNotFoundError because pysal/base.py imported from .frozen which did not exist in the package.
Changes:
Added pysal/frozen.py with frozen_packages that falls back to installed versions when explicit frozen versions are not available
Added tests for Versions.released property and Versions.check() method