GEOPY-2739: Accept BaseUIJson in the start of driver#860
GEOPY-2739: Accept BaseUIJson in the start of driver#860domfournier wants to merge 3 commits intodevelopfrom
Conversation
github-actions
bot
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the UI JSON model naming by renaming the core BaseUIJson class to UIJson, and adjusts tests/imports accordingly to match the new API surface.
Changes:
- Rename
BaseUIJsontoUIJsoningeoh5py.ui_json.ui_json. - Update UI JSON tests to subclass/use
UIJsoninstead ofBaseUIJson. - Update the
geoh5py.ui_jsonpackage export to re-exportUIJson.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| geoh5py/ui_json/ui_json.py | Renames the core model class and updates read() logic/type hints to reference UIJson. |
| geoh5py/ui_json/init.py | Switches the package-level export from BaseUIJson to UIJson. |
| tests/ui_json/uijson_test.py | Updates tests to use UIJson for subclassing/reading. |
| tests/ui_json/forms_test.py | Updates helper test model to subclass UIJson. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @classmethod | ||
| def read(cls, path: str | Path) -> BaseUIJson: | ||
| def read(cls, path: str | Path) -> UIJson: |
There was a problem hiding this comment.
read can return an instance of a subclass when called as MyUIJson.read(...), but the return type is annotated as UIJson. This makes the public API typing inaccurate and can cause mypy/IDE issues. Consider typing it as Self (e.g., cls: type[Self] -> Self) or using overloads to express the UIJson vs subclass behavior.
| """ | ||
| Create a UIJson object from ui.json file. | ||
|
|
There was a problem hiding this comment.
The read method docstring still mentions BaseUIJson (a name that no longer exists after this rename). Please update the wording to UIJson (or remove the class-name reference) so the documentation matches the current API.
|
|
||
| class BaseUIJson(BaseModel): | ||
| class UIJson(BaseModel): | ||
| """ | ||
| Base class for storing ui.json data on disk. | ||
|
|
There was a problem hiding this comment.
Renaming BaseUIJson to UIJson and removing the old symbol is a breaking public API change (it was previously importable from geoh5py.ui_json). If backward compatibility is expected, consider keeping BaseUIJson as a deprecated alias of UIJson (and optionally emitting a DeprecationWarning) to avoid breaking downstream imports.
| from .validation import InputValidation | ||
| from .forms import BaseForm | ||
| from .ui_json import BaseUIJson | ||
| from .ui_json import UIJson |
There was a problem hiding this comment.
This change drops the BaseUIJson re-export from geoh5py.ui_json, which is a breaking import for external callers (from geoh5py.ui_json import BaseUIJson). If the rename is intended, consider re-exporting a deprecated BaseUIJson alias alongside UIJson, or ensure the project versioning/release notes explicitly cover the breaking change.
| from .ui_json import UIJson | |
| from .ui_json import UIJson | |
| # Backwards-compatible alias: BaseUIJson was renamed to UIJson. | |
| # External callers may still import BaseUIJson from geoh5py.ui_json. | |
| BaseUIJson = UIJson |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #860 +/- ##
===========================================
+ Coverage 91.18% 91.21% +0.02%
===========================================
Files 115 115
Lines 10298 10296 -2
Branches 1901 1900 -1
===========================================
+ Hits 9390 9391 +1
+ Misses 485 483 -2
+ Partials 423 422 -1
🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
GEOPY-2739 - Accept BaseUIJson in the start of driver