Multiple Recordings in an Archive #21
Conversation
…igmffile name when reading archives, change error to warning when no sigmf-data files in archive
|
Also note that this fixes a bug where |
|
@gmabey what are your thoughts on this one? |
There was a problem hiding this comment.
I really like most of the changes, but I'm surprised if all the new features are supported in Python 3.6 -- if they are then that's great!
I will try to use this version in my projects and see how well it works in the next couple of days, but in general I'm concerned about a few points of backward compatibility.
@jhazentia could you please try running the tests (maybe you already have) in python 3.6 just to make sure it's good?
| @@ -890,14 +944,23 @@ def get_dataset_filename_from_metadata(meta_fn, metadata=None): | |||
| return None | |||
|
|
|||
|
|
|||
| def fromarchive(archive_path, dir=None): | |||
| """Extract an archive and return a SigMFFile. | |||
| def fromarchive(archive_path): | |||
There was a problem hiding this comment.
I would think it would be better (as it's less likely to break someone's existing code that specifies dir) to leave it as an argument, even if it's not used.
|
|
||
| The `dir` parameter is no longer used as this function has been changed to | ||
| access SigMF archives without extracting them. | ||
| If the archive contains a single recording, a single SigMFFile object will |
There was a problem hiding this comment.
This seems "squishy" to me -- I don't think I'm very comfortable with the prospect of the return type changing based on the contents of the SigMF Archive.
However I do acknowledge that it is an attempt to provide some level of backward compatibility with existing code.
| @@ -511,13 +547,33 @@ def validate(self): | |||
| version = self.get_global_field(self.VERSION_KEY) | |||
| validate.validate(self._metadata, self.get_schema()) | |||
|
|
|||
| def archive(self, name=None, fileobj=None): | |||
| def archive(self, file_path=None, fileobj=None, pretty=True): | |||
There was a problem hiding this comment.
I agree that file_path is truly a better parameter name, but I'm not immediately convinced that it's worth breaking code for someone who provided an explicit parameter name in the method call ...
Thanks for tagging me on this -- this PR had fallen off my radar, and I think I'm currently unsubcribed from changes to this project. |
|
If we want this feature then I'll probably need to dive in and resolve these merge conflicts. Lately been spending free coding time on AoC. |
|
@jhazentia yeah so the reason why the SigMF project has persisted with python 3.6 support is (only because of) RedHat 8 ... that's just the reality of many users (including me), even though it's less than ideal. |
This pull request is intended to add support for multiple recordings in archives. Fixes #11. Please let me know if you have any suggested improvements. Below is a summary of the changes:
SigMFArchiveto accept multipleSigMFFilesSigMFArchivenametopathSigMFArchiveto append to existing tarfile when afileobjis passed that references an existing, open tarfile. Note that, if desired, this can be changed back to always overwrite existing file.prettyparameter toSigMFArchiveto control pretty printing ofSigMFmetadata in archivesnameparameter toSigMFFile.SigMFArchivewill use theSigMFFilenameparameter to create recording parent directories/file names in archiveSigMFArchiveReadernametopathSigMFArchiveReaderto read archives containing multiple recordings. The__len__(),__iter__(), and__getitem__()operate on the list ofSigMFFilesinstead of individualSigMFFileSigMFMetafiledump()anddumps()methods to append newline to the JSON to make this behavior consistent throughout the code.__eq__()method toSigMFFilefor testingNote that this PR does not include any changes to include a collection in archive, read collection from archive, or to update gui.py.
These changes are based on my colleagues' (Douglas Anderson @djanderson and Todd Schumann @ToddSchumann) changes: https://github.com/ntia/sigmf/tree/multi-recording-archive.