os.path converted pathlib.Path #94
Conversation
sigmf/archive.py
Outdated
| @@ -65,9 +65,9 @@ def __init__(self, sigmffile, name=None, fileobj=None): | |||
| sigmf_archive = tarfile.TarFile(mode="w", fileobj=sigmf_fileobj, format=tarfile.PAX_FORMAT) | |||
| tmpdir = tempfile.mkdtemp() | |||
| sigmf_md_filename = archive_name + SIGMF_METADATA_EXT | |||
| sigmf_md_path = os.path.join(tmpdir, sigmf_md_filename) | |||
| sigmf_md_path = Path.joinpath(tmpdir, sigmf_md_filename) | |||
There was a problem hiding this comment.
So whenever we create or handle a path it should be a Path already. So in this example a few lines up it should be tmpdir = Path(tempfile.mkdtemp()), then on this line and the one below you can just do tempdir / sigmf_md_filename.
sigmf/archive.py
Outdated
| @@ -108,7 +108,7 @@ def _ensure_name_has_correct_extension(self): | |||
| has_extension = "." in name | |||
| has_correct_extension = name.endswith(SIGMF_ARCHIVE_EXT) | |||
| if has_extension and not has_correct_extension: | |||
| apparent_ext = os.path.splitext(name)[-1] | |||
| apparent_ext = Path(name).suffix | |||
There was a problem hiding this comment.
And here name will already be a path.
sigmf/schema.py
Outdated
| schema_file | ||
| ) | ||
|
|
||
| schema_path = Path.joinpath(utils.get_schema_path(Path(utils.__file__).parent), schema_file) |
There was a problem hiding this comment.
get_schema_path should be different:
def get_schema_path(version: str=__version__) -> Path:
"""
TODO: Allow getting different schemas for specific SigMF versions
"""
return Path(__file__)so this line would just be schema_path = utils.get_schema_path() / schema_file
There was a problem hiding this comment.
But, get_schema_path() take module_path: str as parameter.
There was a problem hiding this comment.
I wanna ask something. What version is this __version__.
There was a problem hiding this comment.
You can do from . import __version__ which will import the current version string stored in __init__.py.
sigmf/sigmffile.py
Outdated
| @@ -248,7 +248,7 @@ def _is_conforming_dataset(self): | |||
| # check for any non-zero `header_bytes` fields in captures segments | |||
| if capture.get(self.HEADER_BYTES_KEY, 0): | |||
| return False | |||
| if self.data_file is not None and not path.isfile(self.data_file): | |||
| if self.data_file is not None and not Path.is_file(self.data_file): | |||
There was a problem hiding this comment.
similarly self.data_file will already be a Path
|
Tests are also not passing yet. On your machine you should be able to do |
|
I will work on it. But, there are some issue that original codes test not work either. |
sigmf/sigmffile.py
Outdated
| @@ -948,7 +948,8 @@ def get_dataset_filename_from_metadata(meta_fn, metadata=None): | |||
| compliant_filename = get_sigmf_filenames(meta_fn)["data_fn"] | |||
| noncompliant_filename = metadata["global"].get(SigMFFile.DATASET_KEY, None) | |||
|
|
|||
| if path.isfile(compliant_filename): | |||
| if Path.is_file(compliant_filename): | |||
| # if path.isfile(compliant_filename): | |||
There was a problem hiding this comment.
This PR is looking almost ready. You have a few commented lines leftover and I see lots of places where you just need some formatting like a/ b to a / b
|
Try to get all the tests passing too, you can look here in the build log or try on your own machine. If you are stuck let me know and I can try to resolve. |
|
Tests still not passing due to changes needed in I started working on it but didn't complete today. The implementation within that area is more complicated because there is |
|
I'm sorry I couldn't take care of it. Congrats, you did a good job. |
pull request for issue #90. Every path coverted to pathlib. Please, test the code and warn if there is a mistake.