Repository navigation
253 class importer - #258
Conversation
…d more generic file associations
…tself in the generic Importer class
… for a unit test suite
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ps1401
left a comment
ps1401
left a comment
There was a problem hiding this comment.
I have a few questions, but overall this looks solid.
|
|
||
| # ------------------------------------------------------------------------------------------------------------------ | ||
| # Interface to be implemented by every child importer | ||
| # ------------------------------------------------------------------------------------------------------------------ |
There was a problem hiding this comment.
Given they must be implemented, would it be appropriate to make these methods abstract methods?
There was a problem hiding this comment.
This class holds a registry of all child Importers. Making this class abstract breaks some of that convention.
| file = Path(file) | ||
| measurements, errors = self._import_from_url(file, config) | ||
| if file is None: | ||
| break |
There was a problem hiding this comment.
Should this be break or continue? I'd have though if a single file is None, that doesn't affect the entire list, unless you want an error to be raised immediately on finding this. What's the approach here?
There was a problem hiding this comment.
See my change here, but the idea is, if zip_longest finds more configs than files, this should not continue.
| @@ -40,7 +39,7 @@ def test_trend_build_from_xml(): | |||
| and currently a single Metadata (no tree of MetaNodes), but this should be | |||
| corrected in the future in the XML reader, I guess! | |||
There was a problem hiding this comment.
Has this now been corrected, and should the docstring be updated? I know we've got rid of sasentry for hdf5 files.
…les and break out of the loop in such a case
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @classmethod | ||
| def can_import(cls, source: Source_Type) -> bool: | ||
| """The file is HDF5 if the HDF5 format signature is found at offset 0 or at a user block boundary.""" | ||
| if isinstance(source, (str, Path)): |
|
|
||
| def load_data(self, source: Source_Type) -> dict[str, SasMeasurement]: | ||
| """Load a SESANS file. The file holds a single measurement, which is keyed by its name.""" | ||
| if isinstance(source, (str, Path)): |
# Conflicts: # test/data_io/utest_import_ascii.py
…r resolving conflicts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
No quality gates enabled for this code.
See analysis details in CodeScene
Quality Gate Profile: Custom Configuration
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
This applies the common importer class to all importers. File import no longer requires a registry mapping file extension to the reader. Instead, the file contents are examined and an importer is selected that matches the file contents the best.
Fixes #253
Refs #192 - Many of the existing files are now loading, but this will require more testing before I can say it's fully fixed
Disclaimer: Claude was used to help generate the code in the PR.
NB Changes will need to be made in sasview (coming soon) before this should be merged into the refactor branch.