GEOPY-2731#951
Conversation
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #951 +/- ##
==========================================
Coverage ? 91.04%
==========================================
Files ? 129
Lines ? 10905
Branches ? 1996
==========================================
Hits ? 9929
Misses ? 514
Partials ? 462
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the UI JSON core model by renaming BaseUIJson to UIJson, extending serialization / IO to support in-memory BytesIO, and updating tests and validation typing accordingly. It also adjusts how UI JSON can be converted into a UIJsonGroup and broadens form inference strictness to include string values.
Changes:
- Rename the core UI JSON model from
BaseUIJsontoUIJsonand update tests/imports accordingly. - Add
serialize()plusBytesIOsupport forread()/write(), and enable attaching the generated UI JSON asFilenameDataon entities. - Update UI group creation (
to_ui_json_group) option handling and relax a form-type filtering check to treatstrvalues as “strict”.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ui_json/uijson_test.py | Updates tests to use UIJson and adjusts assertions around to_ui_json_group() output. |
| tests/ui_json/forms_test.py | Updates form setup helper test to use UIJson instead of BaseUIJson. |
| geoh5py/ui_json/validation.py | Updates type hints / TYPE_CHECKING imports to reference UIJson. |
| geoh5py/ui_json/ui_json.py | Renames the core model, adds serialize(), BytesIO IO paths, updates to_ui_json_group(), and changes demotion behavior in set_values(). |
| geoh5py/ui_json/forms.py | Expands strict type-checking filter to include strings. |
| geoh5py/ui_json/init.py | Updates package exports to re-export UIJson (previously BaseUIJson). |
| geoh5py/shared/entity_container.py | Extends add_file to accept BytesIO input. |
| .gitignore | Trailing whitespace-only change. |
Comments suppressed due to low confidence (1)
geoh5py/shared/entity_container.py:147
- The
TypeErrormessage is now misleading:add_filealso accepts rawbytes, but the error message only mentions “path or BytesIO”. Update it to reflect the accepted types.
raise TypeError(
f"Input file must be a path or BytesIO object, not {type(file)}"
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
benk-mira
left a comment
benk-mira
left a comment
There was a problem hiding this comment.
Looking good. Couple suggestions/questions. What's the plan to roll this out so that we don't break every package referring to BaseUIJson?
| if validate: | ||
| return ui_json_class(**kwargs) | ||
| else: | ||
| return ui_json_class.model_construct(**kwargs) # type: ignore[return-value, arg-type] |
There was a problem hiding this comment.
Surprised to see in issue with 'return-value', is it not recognizing Self?
|
|
||
| def serialize(self, mode: Literal["json", "python", "str"] | str = "python"): | ||
| """ | ||
| Return a demoted uijson dictionary representation the params data. |
There was a problem hiding this comment.
| Return a demoted uijson dictionary representation the params data. | |
| Return a demoted dictionary representation of the uijson data. |
|
|
||
| return uijson | ||
|
|
||
| def serialize(self, mode: Literal["json", "python", "str"] | str = "python"): |
There was a problem hiding this comment.
| def serialize(self, mode: Literal["json", "python", "str"] | str = "python"): | |
| def serialize(self, mode: Literal["json", "python", "str"] | str = "python") -> dict | str: |
|
|
||
| return self.model_dump(exclude_unset=True, by_alias=True, mode=mode) | ||
|
|
||
| def to_file_data( |
There was a problem hiding this comment.
| def to_file_data( | |
| def attach_file_data( |
| return self.model_dump(exclude_unset=True, by_alias=True, mode=mode) | ||
|
|
||
| def to_file_data( | ||
| self, entity: EntityContainer, name: str | None = None |
There was a problem hiding this comment.
cool, don't think I knew about EntityContainter. It's generic to Objects and Groups?
| ) | ||
|
|
||
| uijson_copy = self.set_values( | ||
| copy=True, **{"geoh5": geoh5, "out_group": out_group} |
There was a problem hiding this comment.
I don't get it.. If self.out_group is not None, this will serialize out_group as a blank UUID instead of existing out_group Form?
No description provided.